Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MDX] Migration to gatsby-plugin-mdx v4 (requires migration to MDX v2) #4532

Merged
merged 69 commits into from
Jul 19, 2023

Conversation

randychilau
Copy link
Contributor

@randychilau randychilau commented Jul 11, 2023

Description

This PR is a migration to gatsby-plugin-mdx v4 (which requires migration to MDX v2)

Motivation: To resolve an issue of imports used in MDX files being added to the app bundle for every page

Unnecessary files in the app bundle adds to the page weight and can potentially cause slower performance and longer loading times, especially on mobile.

  • additional reports:

    Imports in MDX pages go to the main bundle #23345
    MDX app bundle bloating and global scope problems #25069

Gatsby resolves this issue in gatsby-plugin-mdx v4:

Per-file tree shaking and chunking (No more app.js bundle bloat or global scope problems)


Layer5 example

before:
this file src/collections/programs/sca-contributhon/index.mdx imports the image sca.svg
import {ReactComponent as ScaLogo} from "./sca.svg";

when you view the treemap from a recent lighthouse report for the /about page:

image

  1. The sca.svg is included even though it is not displayed on the /about page.
  2. The size of thecollection, much of which shoul not be included in the bundle, is over 300K
    image
  3. This is a significant percentage (27%) of the page weight 1.28Mb, also notice the amount of Unused Bytes which is approx 350kb

after:
image

  1. The collection is significantly reduced and less than 25K
    image
  2. The total page weight is also reduced by 15% to 1.088Mb, and Unused Bytes is approx. 200kb which is over 40% less.

Notes for Reviewers

IMPORTANT:
This PR is stable for creating production builds, but for development builds, it is only stable when including a limited number of mdx files (using ignore option in gatsby-source-filesystem, otherwise there is a significant memory issue in Gatsby v4. Update: Development build is stable, but requires more memory to run.

This memory issue in development may be resolved in Gatsby v5 with an upgraded webpack. However as long as we are using Gatsby v4, it is not possible to get support/feedback from Gatsby Dev team on this issue.

While there are benefits to migrating to MDX v2, the time and conditions required for production and development builds in Gatsby v4 may be too costly to justify migration.

Please note there are multiple aspects of this PR that can be made into separate PRs which do not require upgrading to gatsby-plugin-mdx v4 or mdx v2 and merged with the current site, for example:

  • mdx file changes for MDXv2 compatibility, this shouldn't change how the file will display in the current site using mdx v1.
  • adding eslint settings for MDX to ensure future mdx files will be compatible with MDXv2
  • use of Shortcodes to dry/clean up code.
  • using the ignore option in gatsby-source-filesystem to make local dev and production builds faster, as suggested here.

(please let me know if the above is a desired action)

If the decision is to archive/close this PR until the site migrates to Gatsby 5 (and React 18) and wants to move to MDXv2, then I suggest adding a comment to the gatsby-config.js file in the "gatsby-plugin-mdx" section about this PR for reference and possible future merging.


A OOM memory issue during production builds was resolved using this github comment/workaround in the gatsby-ssr.js file.

A memory issue in the development build was worked around ~~by using github comments on limiting the number of mdx files included ~~(and possibly using gatsby-plugin-no-sourcemaps. Update: reducing the file size of pages built from mdx files helped work around memory issue.

These memory issues may be resolved by migrating to Gatsby v5 because it uses an upgraded version of webpack.


Signed commits

  • Yes, I signed my commits.

Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
Signed-off-by: Randy Lau <randychilau@gmail.com>
@l5io
Copy link
Contributor

l5io commented Jul 19, 2023

🚀 Preview for commit 697d013 at: https://64b77eab68eede4ef5ccdc34--layer5.netlify.app

@randychilau
Copy link
Contributor Author

Hi @Nikhil-Ladha,

Thanks for the kind words. Sounds good, I updated the CONTRIBUTING.md doc with info on the build times in "How to Contributes" section and added guidelines about image filesize & dimensions, feel free to review/edit.

I recently found that any component used for shortcodes in mdx files were added to the general app bundle for all pages. So even though the page weight decreased with the bloating issue resolved, it was countered by the components (and related libraries) added via the shortcodes. This seems to be resolved by using loadable to have the shortcode components lazy load in root-wrapper.js.

For example, the following data is for the home page:

with PR:
image

current live site:
image

notes:

  • I removed the gatsby-plugin-no-sourcemaps to be able to see the treemap in Lighthouse, however then the production build started crashing again, so I reinstalled the plugin.
  • As more pages and images get added to the site, we'll get closer to the trigger for memory crashes. An optimization audit and clean up of unused files, libraries should be scheduled soon.

Regarding the upgrade migration to Gatsby v5, I am interested in helping with this if that is the desired direction. I noticed you were responsible for upgrading the site to Gatsby v4. Do you have any advice for going through this process?

Lastly, thanks for taking the time overseeing and reading through this verbose PR.

Cheers,
Randy

Copy link
Contributor

@Nikhil-Ladha Nikhil-Ladha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great, let's merge this and let things settle in.

@Nikhil-Ladha
Copy link
Contributor

Nikhil-Ladha commented Jul 19, 2023

Hi @Nikhil-Ladha,

Thanks for the kind words. Sounds good, I updated the CONTRIBUTING.md doc with info on the build times in "How to Contributes" section and added guidelines about image filesize & dimensions, feel free to review/edit.

I recently found that any component used for shortcodes in mdx files were added to the general app bundle for all pages. So even though the page weight decreased with the bloating issue resolved, it was countered by the components (and related libraries) added via the shortcodes. This seems to be resolved by using loadable to have the shortcode components lazy load in root-wrapper.js.

Yeah, I am aware of this. Though, just to confirm did you cross-check if we were importing a component in an incorrect way that is leading to this, or that's just how Gatsby is? I think it's the latter, but WDYT?

For example, the following data is for the home page:

with PR: image

current live site: image

notes:

  • I removed the gatsby-plugin-no-sourcemaps to be able to see the treemap in Lighthouse, however then the production build started crashing again, so I reinstalled the plugin.
  • As more pages and images get added to the site, we'll get closer to the trigger for memory crashes. An optimization audit and clean up of unused files, libraries should be scheduled soon.

I can feel the same too, we might need some optimization throughout the code base in how we are using GraphQL as well.

Regarding the upgrade migration to Gatsby v5, I am interested in helping with this if that is the desired direction. I noticed you were responsible for upgrading the site to Gatsby v4. Do you have any advice for going through this process?

Well, thanks to Gatsby documentation it makes the process much smoother. Just follow the migration guide, there might be some hiccups due to some extra plugins that we use, but we should be able to get around it some way or the other. Let me know, if something doesn't pan out, we can connect and discuss futher on that.

Lastly, thanks for taking the time overseeing and reading through this verbose PR.

You're welcome :)
We need more contributors like you to keep the spirit of open-source up and alive 💯

Cheers, Randy

@randychilau
Copy link
Contributor Author

Hi @Nikhil-Ladha,

As you can see, things did not go as planned, and turned into one of my biggest worries as an aspiring developer. Thankfully it was reverted relatively quickly.

What I neglected to consider was that anything involving page creation needs another layer of testing with the CI variable set to true, since we are customizing the handling of urls for GitHub Pages.

Another misstep was that I should have taken a look at the build logs and tested the netlify site preview, instead of simply trusting the checks passing and my experience with the local production builds. Then I would have:

  • seen the .html.html in the build logs:
    image

  • noticed the redirect loop in the netlify site preview.

I think what is happening with the gatsby-plugin-mdx v4 when CI==="true" is that it is triggering our customized onCreatePage function which is meant to handle non-mdx sourced pages. This was not the case in v3, and in the doc it says:

There is a mechanism in Gatsby to prevent calling onCreatePage for pages created by the same gatsby-node.js to avoid infinite loops/callback

The mdx source pages are being created by the same gatsby-node.js so it should not trigger onCreatePage, but perhaps the plugin is doing something different now (it only does this when using ${postTemplate}?__contentFilePath=${node.internal.contentFilePath} value in the component property, it behaves as previously when only the template is used).

To handle this trigger in onCreatePage, a conditional check is done to see if the path ends in .html, and then it simply returns. This seems to have resolved the issue, but cannot be absolutely sure until it is in a live GitHub Pages production environment. The PR is pending since I had some other missteps with git.

Is there a GitHub Page testing environment that can be used for PRs that involve page creation as a double check before going to live production?

Anyway, hopefully that fix will resolve it and things can move forward.

Cheers,
Randy

@Nikhil-Ladha
Copy link
Contributor

A big miss from my end too, should have cross checked the preview :/
Anyway, it seems like the internal working of the plugin has changed as you pointed out and seems to be interferring with how we are handling the trailing slashes.
Thank you for being on top of it.

@Nikhil-Ladha
Copy link
Contributor

Regarding the testing env, we created a testing repo long back, probably you can use that.
It uses Github pages to serve the site so we should be able to test it out correctly there itself.

@randychilau
Copy link
Contributor Author

Hi @Nikhil-Ladha,

The testing repo looks very interesting, a few questions:

  • are the settings already configured to publish from a branch?
  • should the master be made current with the live site? or unnecessary?
  • what are the steps for uploading/merging a PR to it for site preview viewing?

Structure of pull request previews:

https:layer5labs.github.io/layer5/<pr#>

I think this would be very useful for changes to gatsby-node.js involving page creation/redirects/urls.

Thanks,
Randy

@Nikhil-Ladha
Copy link
Contributor

The site is already configured to build from Github pages, so once I merge your PR in the repo you should be able to see the same on the hosted site.
We don't have a preview mechanism as such in the repo.

@leecalcote
Copy link
Member

Ah, geez. I was to help, but time got away from me. Did we end up getting @randychilau's PR/commits reopened? Is that what this PR is?

@leecalcote
Copy link
Member

Maybe it's evident or has already been discussed, but outside of local builds, deployment previews are build each time that a PR is opened and each time that a new commit is pushed to an open PR.

@Nikhil-Ladha
Copy link
Contributor

Maybe it's evident or has already been discussed, but outside of local builds, deployment previews are build each time that a PR is opened and each time that a new commit is pushed to an open PR.

We are talking about the configuration available in the laye5labs/layer5 repo, a secondary source to test out the site changes using Gtihub pages.

@leecalcote
Copy link
Member

Oh! Gotcha. Great suggestion, particularly, since @Nikhil-Ladha is quite adept at X-Robots-Tag: noindex. 😄

@Nikhil-Ladha
Copy link
Contributor

Oh! Gotcha. Great suggestion, particularly, since @Nikhil-Ladha is quite adept at X-Robots-Tag: noindex. 😄

Haha, I see that 🌚

@randychilau
Copy link
Contributor Author

@leecalcote No problem, I appreciated the quick response and offer to help. Thanks to a blog post on git reverting reverts, I was able to get another PR set up (#4585) for review.

Regarding previews, I thought for PRs that specifically interact with the trailing slash customizations for GitHub pages, an option to test a preview served by GitHub Pages may be useful (since the current site previews in GitHub Actions are served by Netlify).


@Nikhil-Ladha When you can, please merge the PR to the testing repo to see it in action.

I can also update the CONTRIBUTING.md "Developer Notes" section with some language about this repo being available for additional testing of certain PRs and asking a maintainer for additional assistance/more information.

Thanks!

@Nikhil-Ladha
Copy link
Contributor

@leecalcote No problem, I appreciated the quick response and offer to help. Thanks to a blog post on git reverting reverts, I was able to get another PR set up (#4585) for review.

Regarding previews, I thought for PRs that specifically interact with the trailing slash customizations for GitHub pages, an option to test a preview served by GitHub Pages may be useful (since the current site previews in GitHub Actions are served by Netlify).

@Nikhil-Ladha When you can, please merge the PR to the testing repo to see it in action.

Sure, some more changes are required in the repo before the testing because of the repo confiuration like adding a prefix to the gatsby-confg. I am a bit occupied as of now, so do you mind testing it out yourself? If so, signal me here and I would grant you merge permissions to the repo and you can test it out by mergeing the changes directly and testing out. No PR previews are available there, sorry 😓

I can also update the CONTRIBUTING.md "Developer Notes" section with some language about this repo being available for additional testing of certain PRs and asking a maintainer for additional assistance/more information.

I would let it be a secret, and only grant access to special scenarios :)

Thanks!

@randychilau
Copy link
Contributor Author

@Nikhil-Ladha Sure, please grant me merge permissions and I can try testing it out myself (at least if I start a 🔥 on this site, it won't required emergency assistance 🚒 🧯). Thanks for taking the time!

@Nikhil-Ladha
Copy link
Contributor

Cool, sent you the invite.
For your ref: https://www.gatsbyjs.com/docs/how-to/previews-deploys-hosting/path-prefix/ this is what I am talking about.
Check the deployment link under Settings tab of the repo, if in case it is not showing here's the link: https://layer5labs.github.io/layer5/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/blog New posts or new blog functionality area/careers area/community area/events area/learn Related to /learn section area/news A noteworthy article, event, happening area/projects An issue relating to Layer5 initiatives (projects) area/resources area/site-config project/kanvas project/meshery
Development

Successfully merging this pull request may close these issues.

4 participants