Update to CRA 5 #4802
Update to CRA 5 #4802
Conversation
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
✅ Deploy Preview for kubeapps-dev canceled.
|
| @@ -122,7 +124,7 @@ | |||
| "typescript": "^4.7.2" | |||
| }, | |||
| "resolutions": { | |||
| "@babel/parser": "^7.15.3", | |||
| "@babel/parser": "^7.18.4", | |||
There was a problem hiding this comment.
Quick reminder (as I also had to think twice): this resolutions object is there to prevent react 18 to be selected by default. Our app is not still prepared to run React 18.
Just upgrading to the latest minor version.
There was a problem hiding this comment.
Thanks for the explanation here - sounds like it could be worth a comment in the file at this point to our future selves?
There was a problem hiding this comment.
Unfortunately, yarn/npm doesn't accept comments as part of the JSON file (even if jsonc exists...). See https://stackoverflow.com/questions/14221579/how-do-i-add-comments-to-package-json-for-npm-install to see some hacky ways to work around it.
Not sure if we should do something like that, but happy to explore some ways anyway in next PRs.
| webpack: { | ||
| configure: { | ||
| plugins: [ | ||
| new NodePolyfillPlugin(), // add the required polyfills (not included in webpack 5) |
There was a problem hiding this comment.
This is preventing us from doing something like:
fallback: {
process: require.resolve("process/browser"),
stream: require.resolve("stream-browserify"),
buffer: require.resolve("buffer"),
timers: require.resolve("timers-browserify")
... // a line per each polyfill we (not really "we" but any random transitive dependency we use) need
},
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
| let wrapper: any; | ||
| await act(async () => { | ||
| wrapper = mountWrapper(defaultStore, <AppRepoForm {...defaultProps} repo={repo} />); | ||
| }); | ||
| await waitFor(() => { | ||
| wrapper.update(); | ||
| expect(wrapper.find("textarea").at(0).prop("value")).toBe("nginx"); |
There was a problem hiding this comment.
Even if the tests were passing, they were throwing a message like (taken from any green CI step)
Since mounting AppRepoForm triggers an state update via useEffect in an async fetch function, it has to be wrapped into an act. Then, a .update() call is often required.
I've performed this change in the remaining test cases.
There was a problem hiding this comment.
Excellent thanks. Yeah, we'd started using act for async tests in other places, but only as errors were raised in tests. Thanks for doing them all here.
| let wrapper: any; | ||
| await act(async () => { | ||
| wrapper = mountWrapper(defaultStore, <AppRepoForm {...defaultProps} repo={repo} />); | ||
| }); | ||
| await waitFor(() => { | ||
| wrapper.update(); | ||
| expect(wrapper.find("textarea").at(0).prop("value")).toBe("nginx"); |
There was a problem hiding this comment.
Excellent thanks. Yeah, we'd started using act for async tests in other places, but only as errors were raised in tests. Thanks for doing them all here.
Description of the change
Today, I performed a
yarn installto update my local deps with the latest dependabot updates. However, I got an error related to webpack 4 not being able to parse some new(ish) JS features like nullish coalescing or optional chaining. The offending dependency was the latest version ofyamldependency that we recently updated.Instead of work around this issue, I decided to invest some time to look into #4018.
This PR updates to CRA >=5 adding the usual workaround I mentioned in the aforementioned issue: adding the polyfills manually.
However, instead of adding them independently, I've added
node-polyfill-webpack-plugin, which should add the required ones more transparently.The webpack tweak is performed using
cracoa tool to inject some webpack configuration without ejecting thereact-scriptsway.Benefits
We will get rid of many security notices related to CRA < 5.
Possible drawbacks
Not "pure" react-scripts way, but... using
cracois way less intrusive than ejecting.Applicable issues
Additional information
Working locally, but let's see if our CI thinks otherwise :P
Edit: it seems some eslint rules have been added, I'll fix the offending lines in the tests and will push again. DONE