layer5 icon indicating copy to clipboard operation
layer5 copied to clipboard
Published 2 months ago • verified publisher icon layer5io

[MDX] Migration to gatsby-plugin-mdx v4 with fix for onCreatePage trigger issue

Open randychilau opened this issue 2 years ago • 25 comments

Description

This PR is a second try migrating to gatsby-plugin-mdx v4 (and MDXv2) which fixes an issue discovered in the first try (PR #4532).

The issue was that mdx pages were being handled differently in v4 when the env variable CI is set to true activating customized onCreatePage and envCreatePage functions. CI=true is the default case for all Github Actions.

As a result, mdx sourced pages which used the children feature of v4 were triggering the customized onCreatePage function which is designed to handle non-mdx sourced pages. Adding a conditional check for path ending in .html resolves the issue.

before (taken from the checks in PR 4532): image

redirect loop example at /programs/gsoc in netlify site preview.

after (taken from the checks in this PR): image

no redirect loop example at /programs/gsoc in netlify site preview.

Notes for Reviewers

Added section to CONTRIBUTING.md file with reminders when working on gatsby-node.js and page creation/url/redirect.

:hibiscus: Thanks for everyone's patience and quick action to report issues, revert the PR, and discuss possible solutions.

Signed commits

  • [x] Yes, I signed my commits.

randychilau avatar Jul 20 '23 05:07 randychilau

🚀 Preview for commit d8e08aeb200ef7efd7f13d7cf4651f91b1551cb4 at: https://64b8cdb5900f064ab12c3be6--layer5.netlify.app

l5io avatar Jul 20 '23 06:07 l5io

Everything looks, actually good this time. Godd catch!

Nikhil-Ladha avatar Jul 20 '23 06:07 Nikhil-Ladha

🚀 Preview for commit b8ec8f5fc27e2e106d1041e8fa03015f08445c56 at: https://64bac90b210ccf0508d788eb--layer5.netlify.app

l5io avatar Jul 21 '23 18:07 l5io

🚀 Preview for commit cb31caa7c481339e071f3694e4fcacb62c1145a7 at: https://64bad58a9338e10375d87a77--layer5.netlify.app

l5io avatar Jul 21 '23 19:07 l5io

🚀 Preview for commit e4c1b8f4928dc22a8a6d990d8c5dfa2d39ad84ab at: https://64baf5fbcf89bc03a5636f85--layer5.netlify.app

l5io avatar Jul 21 '23 21:07 l5io

Please do discuss the PR on the Monday's website call if possible for you (time wise), and request for people to test out the preview. From, my side things look good and we can give this a go.

Nikhil-Ladha avatar Jul 22 '23 12:07 Nikhil-Ladha

🚀 Preview for commit c594161d76a3441e8d9a6fa3e5014211e37d6447 at: https://64bc318dc252e0225e126f4e--layer5.netlify.app

l5io avatar Jul 22 '23 19:07 l5io

Hi @Nikhil-Ladha,

We discussed this PR on today's Website's call, and asked the community to test the preview and I believe someone also built the branch without issues.

@leecalcote mentioned that this merge might be an issue for contributors who have experienced memory issues during the image-processing phase. Something I forgot to mention was that the memory issues were occurring during the Build HTML Renderering stage, and there were generally no issues during the image-processing stage. However every contributor's setup is different, and since I have not had issues with image-processing in the past, my build data points may be irrelevant.

Lee also mentioned potentially migrating to Gatsby 5 (which requires bump React 18 and includes a new webpack version) in this PR to see if the cold build time will come down.

So as of now I see a few options:

  1. merge this PR

    • accept the longer cold build time for now
    • listen for any memory build issues from community
    • create separate PR for migration to Gatsby 5

  2. add to this PR a Gatsby 5 migration

    • test to see build time reduces in Gatsby 5
    • re-evaluate whether or not to merge this PR

  3. hold this PR and create separate PR for migration to Gatsby 5 on the current site

    • after PR for Gatsby 5 migration is tested and merged, revisit this PR for testing

My initial thought is if the choice is between option 2 or 3, then 3 would be the way to go since a Gatsby 5 migration can be done independent of MDX related upgrades and has its own benefits for the site.

Happy to hear thoughts on the above and how to proceed.

Cheers, Randy

randychilau avatar Jul 24 '23 19:07 randychilau

Even I would vote for 3, because adding the Gatsby v5 migration to this PR itself, would make it a very big PR, which should be avoided.

@randychilau If you are ready to start working on Gatsby v5 migration and see if it actually helps in resolving memory issue, then I am fine with it.

It's just that keeping such a big PR on hold for so long increases the PR owner's effort of rebasing it again and again, but if you are ok with doing that, then I don't mind and we can give v5 a chance.

Nikhil-Ladha avatar Jul 25 '23 03:07 Nikhil-Ladha

Hi @Nikhil-Ladha,

Thanks for the feedback.

Yes, I am okay with keeping this PR on hold until a Gatsby 5 migration is merged.

I will start working on Gatsby 5 migration and submit it as separate PR.

Cheers, Randy

randychilau avatar Jul 25 '23 16:07 randychilau

Merge conflicts @randychilau

Chadha93 avatar Aug 23 '23 20:08 Chadha93

Hi All,

Apologies for the inactivity or lack of updates regarding this PR. I took a AFK vacation to refresh, but unfortunately did not leave a note letting anyone know.

Hopefully this PR will still be helpful to the project if the PR upgrade to Gatsby v5 is successful (where I am focusing my energies now).

Thanks again for everyone's patience.

Cheers, Randy

p.s. Thanks @Chadha93 for the note!

randychilau avatar Aug 24 '23 16:08 randychilau

🚀 Preview for commit 39d8eae3c2ae906c097a735d0d7456d064d4c3ba at: https://64f3853fd2209b2234574ae0--layer5.netlify.app

l5io avatar Sep 02 '23 18:09 l5io

🚀 Preview for commit 5d77acb7cf58e446018cfdb170880b6c1b6233cd at: https://64f3b48cd019ec35a3374272--layer5.netlify.app

l5io avatar Sep 02 '23 22:09 l5io

🚀 Preview for commit a80777ed2c88c3382ab6493fee6d4e8dbf02c4c5 at: https://64f3c0597209f038eab91b4e--layer5.netlify.app

l5io avatar Sep 02 '23 23:09 l5io

🚀 Preview for commit 7eddcd5d9ccdda4729ab1377e88eeb73555a4281 at: https://64f6c9073e38135fb2118291--layer5.netlify.app

l5io avatar Sep 05 '23 06:09 l5io

🚀 Preview for commit 1e10f7afa99a41a3a76426c8e79ae285cfcb7a49 at: https://64f774de721d0f1471d942f0--layer5.netlify.app

l5io avatar Sep 05 '23 18:09 l5io

🚀 Preview for commit d4861ea7f019e2795769c42fd51f732c2a8742f3 at: https://64f77a7a28f3e016e67a88f9--layer5.netlify.app

l5io avatar Sep 05 '23 19:09 l5io

🚀 Preview for commit 3db10706d44f0794eaf9e71a85a2ef5b9f469df1 at: https://64f79759bfd6992f58637054--layer5.netlify.app

l5io avatar Sep 05 '23 21:09 l5io

🚀 Preview for commit d64cd34bc6b7298e6c6e8244aa1e8534ebf1bb5f at: https://64f7a158767e580074fd3467--layer5.netlify.app

l5io avatar Sep 05 '23 21:09 l5io

Hi All,

Reviewed the latest updates/changes for compatibility with MDX v2, and added them to this PR.

This PR is waiting on the review and merging of PR #4646 (Gatsby v5). Afterwards, will merge those changes into this PR with the hope that migrating to Gatsby v5 (and new webpack) will address cold build times and memory requirements with MDX v2.

Thanks, Randy

randychilau avatar Sep 05 '23 23:09 randychilau

🚀 Preview for commit 6245012cffe62b9135699809cba05cc76f39a7f4 at: https://64ff5ed6ecfe833e585869b2--layer5.netlify.app

l5io avatar Sep 11 '23 18:09 l5io

🚀 Preview for commit 228a0e4a78c6b8072259a38d3a743db16ae71c57 at: https://650b43fdbb800d32b676c3a6--layer5.netlify.app

l5io avatar Sep 20 '23 19:09 l5io

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Mar 17 '24 14:03 stale[bot]

@Aviral0702

vishalvivekm avatar Jul 29 '24 13:07 vishalvivekm