Enable support for React 18, bump Gatsby to v5 #3367
Conversation
|
Thanks for the pull request, @bradenmacdonald! This repository is currently maintained by @openedx/paragon-working-group. Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.
|
openedx-webhooks
added
the
open-source-contribution
✅ Deploy Preview for paragon-openedx-v23 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
After reading through a ton of gatsbyjs/gatsby#36899 and trying a few things I found gatsbyjs/gatsby#36899 (comment) and managed to get the dev server running. bsmith@aximdev:~/code/paragon$ git diff
diff --git a/www/gatsby-config.mjs b/www/gatsby-config.mjs
index d0ab6c658e..aaf5daab39 100644
--- a/www/gatsby-config.mjs
+++ b/www/gatsby-config.mjs
@@ -130,4 +130,7 @@ export default {
// Match the location of the site on github pages if no path prefix is specified
pathPrefix: process.env.PATH_PREFIX || '',
plugins,
+ flags: {
+ DEV_SSR: true
+ },
};
Edit: I looked into this a bit more and found https://www.gatsbyjs.com/docs/reference/release-notes/v2.28/#feature-flags-in-gatsby-configjs. Setting the |
|
Re:
With this change - default: require.resolve(
- './src/templates/default-mdx-page-template.tsx',
- ),It seems the |
✅ Deploy Preview for paragon-openedx-v22 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Yeah, me too. I tried a couple things based on the docs but didn't have much luck yet. There is some perhaps complication from the fact that Gatsby includes everything in Clearly, there is somewhere that we need to specify that |
|
So I managed to get the layout page to render, I had to comment some stuff in the |
|
@brian-smith-tcril Nice! I pulled in most of that change for now. It seems like pages like |
|
For some reason, either when I changed the target branch to |
|
@bradenmacdonald the build step of the latest deploy of this branch on netlify failed. I pulled the latest version of this branch and tried running I commented out a couple things in |
|
Ah, thanks @brian-smith-tcril :) I'll hold off on committing that for now until I see if we can actually get that |
|
re:
We discussed this in the WG meeting yesterday. It seems the version bump is a minor version bump as opposed to a major one:
If there's some nuance here I'm missing please let me know! |
|
@brian-smith-tcril FYI, I got the https://deploy-preview-3367--paragon-openedx-v22.netlify.app/ |
bradenmacdonald
force-pushed
the
braden/react-gatsby-bump
branch
from
c85a647 to
7fbf83e
Compare
|
@brian-smith-tcril When I pulled in the changes from #3125 into this branch, I encountered the same problem with webpack confusing two files like So what I've done for now is renamed e.g. |
That sounds like a very sensible solution to me! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## release-22.x #3367 +/- ##
================================================
- Coverage 93.49% 93.44% -0.05%
================================================
Files 252 251 -1
Lines 4655 4519 -136
Branches 1088 1016 -72
================================================
- Hits 4352 4223 -129
+ Misses 296 293 -3
+ Partials 7 3 -4 ☔ View full report in Codecov by Sentry. |
|
@bradenmacdonald I see there are only a couple things left unchecked:
I'd be more than happy to dive in and try to figure some of those out - is there one in particular you'd like another set of eyes on? |
|
@brian-smith-tcril I don't know much about the Playground page, so that would probably be the one I could most use help with - thanks! |
There was a problem hiding this comment.
Thanks for tackling these changes, @bradenmacdonald! Left a few comments throughout. Here's a couple other issues I noticed when comparing this PR's deploy preview vs. the v22 docs site:
- Modals seem to be throwing a JS error on the docs site when trying to open them. For example, see
AlertModal(link) and click the "Open alert modal" button. Observe a JS errorTypeError: object is not iterable (cannot read property Symbol(Symbol.iterator)). This seems to occur on each modal variant that extendsModalDialog. - The
useIsVisiblehook (link) doesn't seem to be working properly anymore. The "Is element visible? yes/no" in the live code example is not reflecting whether the element is actually visible or not (consistently says "no" even when the element is visible).
| @@ -17,14 +17,10 @@ | |||
| "license": "ISC", | |||
| "dependencies": { | |||
| "@edx/brand": "npm:@openedx/brand-openedx@^1.2.2", | |||
| "@edx/frontend-platform": "^4.2.0", | |||
| "@edx/frontend-build": "^13.0.14", | |||
There was a problem hiding this comment.
[clarification] Why is @edx/frontend-build moved to a regular dependency vs. keeping it as a dev dependency?
There was a problem hiding this comment.
This is for the example MFE (not Paragon itself), and I'm following the best practice I established for MFEs that "dependencies" are what you need to build it (before deploying), and "devDependencies" are for development-only things like tests and linting.
If we had a part of the MFE that runs on the server, it would be different, but there's no server-side code here.
www/src/components/CodeBlock.tsx
Outdated
| if (className === '' && typeof children === 'string' && !children.includes('\n')) { | ||
| // This is an inline code block. Don't use syntax highlighting. | ||
| return <code>{children}</code>; | ||
| } |
There was a problem hiding this comment.
[curious] When/where does this conditional for an inline code block get used?
There was a problem hiding this comment.
Anywhere you have an `inline code node` like this:
There was a problem hiding this comment.
(I shouldn't have used the word "block")
There was a problem hiding this comment.
Ah, I see. Makes sense, though somewhat odd that on master, the CodeBlock was not previously used for these inline code nodes, despite having code: CodeBlock configured within the previous implementation's shortCodes (source). For example, adding a
With CodeBlock actually being used now for these inline code nodes, makes sense we'd need to handle it explicitly. Wonder why it wasn't used before. Either way, thanks for clarifying!
There was a problem hiding this comment.
Yeah, I am also wondering why. Those inline nodes are still using code as their tag before this PR, so it's unclear to me why they weren't using CodeBlock. In any case, it was an easy workaround.
Ah, these were actually two more symptoms of the same general problem of webpack getting confused between |
|
|
Tested both |
|
I'm seeing the same in |
|
@brian-smith-tcril @adamstankiewicz Alright, from my perspective this PR is ready to go! Could I please get a review/approval from you folks? And then (assuming no further issues arise), let me know how/when we should merge this. I'm thinking it will be good to preserve some of the commit history for future reference and to make it easier to deal with porting to the v23 branch, but I can definitely squash some things down. |
| @@ -50,6 +50,8 @@ class Modal extends React.Component { | |||
|
|
|||
| componentWillUnmount() { | |||
| if (this.parentElement) { | |||
| // TODO: update this to use the new createRoot() compatible APIs. | |||
There was a problem hiding this comment.
If this TODO is one that's going to stick around after this PR lands it'd probably be good to make a GH issue for it
There was a problem hiding this comment.
Created issue: #3382
|
@bradenmacdonald re:
I agree keeping the history here could be nice, but I'm also not opposed to squashing the whole thing. There are definitely at least a couple commits that should be squashed if we keep the history ( edit: looks like this would require some fancy rebasing to keep history edit2: it looks like the "you can't rebase" message is because of 505c4ba (merge commit). With that in mind I'm leaning towards just squashing this in and referencing the PR when porting to 23. I'm not opposed to the 23 history having:
But I'm very open to other opinions on the matter. |
bradenmacdonald
force-pushed
the
braden/react-gatsby-bump
branch
2 times, most recently
from
c88dd37 to
ecb828b
Compare
@brian-smith-tcril I've never been afraid of fancy rebasing :p I've now squashed this down to 6 commits and I think each of them makes sense, but I can squash them further if you want. The diff is exactly identical except I made one tiny correction while I was at it: |
|
I just noticed that some component pages like "Marketing Modal" were missing the SCSS variables that they're supposed to show, so I fixed it with b4a4bb3. I'm going to squash that commit into the other big commits though. |
Includes:
* misc gatsby GraphQL syntax updates
* fix markdown syntax issues ('<' and '{' now require escaping)
* convert gatsby-config to ESM
* add 'rehype-mdx-code-props' plugin to get live code blocks working
* fix: "Do not define components during render" warning caused by PropsTable component that's only used in one place (duplicate props tables from "data view" page). Fixed by removing the duplicate tables.
* set Content-Security-Policy header so playground works again
* used sass-migrator to auto-fix use of deprecated global functions like mix(), map-merge() * used sass-migrator to auto-fix things like @media query syntax * suppress other SASS deprecation warnings that we cannot fix at this time * update stylelint rules
bradenmacdonald
force-pushed
the
braden/react-gatsby-bump
branch
from
b4a4bb3 to
271ef2e
Compare
Description
We are long overdue for supporting React 18. The challenge is that bumping to React 18 and testing with it on our docs site also requires bumping Gatsby from v4 to v5 and
gatsby-plugin-mdxfrom v3 to v5, which introduces a ton of breaking changes to how Gatsby and MDX work.These upgrades have been attempted before in #2774 and #2767 but so far nobody has been able to finish the work up. I'm thinking we need to bite the bullet sooner or later and get it sort out.
Deploy Preview
https://deploy-preview-3367--paragon-openedx-v22.netlify.app/
Note: Compare to https://paragon-openedx-v22.netlify.app , not to https://paragon-openedx.netlify.app/ !
Status
Working:
npm run build)Issues and solutions
useIsVisiblehook (link) doesn't seem to be working properly anymore.iconsare showing a lot of babel compatibility boilerplate again, i.e. the changes from Reduce dist size by 500 KB - reinstate browserslist, removing transforms for ancient browsers #3284 seem to be undone after upgrading to React 18.@edx/browserslist-configto v1.3.0 to avoid Update our browserslist config per "Best Practices" browserslist-config#16 which seems to be the source of the problemjsx livearea that is just appearing as plain code, not rendered components.pageQuerydata - specifically "Layout" and "CSS Utilities" , have errors and aren't loading properly. Fixed with aed4ef4 and 8abf42cnpm run start. I thought I had solved this withGATSBY_CPU_COUNT=4(f67d1f7) but it no longer seems to be helping (even if lowered down to 1). I think the memory usage depends on how much has been cached, so clearing the cache always requires a lot more memory. For now the only workaround is to run it withNODE_OPTIONS=--max-old-space-size=16384 npm run startbut allocating 16GB each time is not feasible.npm run build-docsseems to be working fine, and the "deploy preview" build of this PR works fine too.DEV_SSR/FAST_DEVwill avoid the issue.sassused via transitive dependencies. Will this version bump cause issues for Paragon consumers (if we change the sass code to the new version)? e.g. see 958f617sassmeans instead of e.g.map-get(...)we are encouraged to use@use "sass:map";andmap.get(...)- done via 958f617 but this may need to be reverted, see previous point.importetc. have been suppressed for now: 6946d67 we probably can't address this properly without a major bootstrap upgrade{and<characters must now be escaped in markdown - solved with 0f6004fchildMdxAPI is no longer available -gatsby-plugin-mdxno longer allows rendering multiple MDX documents in one page. I worked around this by addingreact-markdownand rendering the props descriptions as plain markdown (they don't use MDX anyways) - 48aa670liveautomatically, so I had to addrehype-mdx-code-propsin 11b7605<PropsTable>as an MDX component was tricky because it requires some context from the parent JSX component, which was being inlined as a local variable resulting in lint warnings: "Do not define components during render". It turns out that the props tables in question, on this Data Views page are already present in the usual place at the end of the document anyways. So I think it's more consistent to just remove support for this element from that page (remove the duplicate props tables) and avoid having to re-engineerPropsTableto better support context for this one very questionable use case. Done with c92a1a2