react imports cleanup 1 #3840
react imports cleanup 1 #3840
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @jacobo-dominguez-wgu! This repository is currently maintained by 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. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
✅ Deploy Preview for paragon-openedx-v23 ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-23.x #3840 +/- ##
================================================
+ Coverage 94.34% 94.37% +0.03%
================================================
Files 242 242
Lines 4276 4282 +6
Branches 1017 1017
================================================
+ Hits 4034 4041 +7
+ Misses 234 233 -1
Partials 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jacobo-dominguez-wgu
added
the
mao-onboarding
jacobo-dominguez-wgu
requested review from
PKulkoRaccoonGang and
brian-smith-tcril
brian-smith-tcril
left a comment
brian-smith-tcril
left a comment
There was a problem hiding this comment.
Overall this looks great!
I left a couple comments with questions but nothing major.
Thanks for doing this cleanup!
| return ( | ||
| // eslint-disable-next-line jsx-a11y/label-has-for | ||
| <label | ||
| id={`label-${this.state.id}`} | ||
| htmlFor={this.state.id} | ||
| className={classNames({ | ||
| 'form-check-label': this.isGroupedInput(), | ||
| })} | ||
| > | ||
| {this.props.label} | ||
| </label> | ||
| ( | ||
| <label | ||
| id={`label-${this.state.id}`} | ||
| htmlFor={this.state.id} | ||
| className={classNames({ | ||
| 'form-check-label': this.isGroupedInput(), | ||
| })} | ||
| > | ||
| {this.props.label} | ||
| </label> | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Is there a specific reason the second set of ( ) is being added here?
There was a problem hiding this comment.
I think this was added by the linter, let me remove it.
src/Badge/index.jsx
Outdated
| @@ -1,8 +1,8 @@ | |||
| import React from 'react'; | |||
| import React, { forwardRef } from 'react'; | |||
There was a problem hiding this comment.
Does removing React not work here? If it doesn't then could you add a comment explaining what happens if you try removing it?
There was a problem hiding this comment.
Comment added
src/Button/index.tsx
Outdated
| import type { | ||
| ElementType, ComponentType, ReactNode, ForwardedRef, AriaRole, | ||
| } from 'react'; | ||
| import React, { forwardRef } from 'react'; |
There was a problem hiding this comment.
Does removing React not work here? If it doesn't then could you add a comment explaining what happens if you try removing it?
There was a problem hiding this comment.
Comment added.
| "presets": [ | ||
| ["@babel/preset-env", { "modules": false } ], | ||
| ["@babel/preset-react", { "useSpread": true } ], | ||
| ["@babel/preset-react", { "useSpread": true, "runtime": "automatic" } ], |
There was a problem hiding this comment.
Are these needed for this PR? If not, I think it makes sense to split this change out into a separate PR.
There was a problem hiding this comment.
Actually it is only needed the one for the test env.
There was a problem hiding this comment.
It feels very odd to me that this would only be an issue when running tests and not when using Paragon installed in an MFE.
I tested this locally by pulling this branch (dccbbdf as of writing) and running npm run build to get a dist directory, then configuring module.config.js (renamed to module.config.mjs) in the authoring MFE (which uses ActionRow on the homepage) to use Paragon from that dist directory and everything seemed to still work.
In general the fact that we have a different babel config for our tests than the standard build is a bit concerning to me.
I'd like to see if anyone else has thoughts on this before moving forward with it. @diana-villalvazo-wgu, @bradenmacdonald, @PKulkoRaccoonGang, @wgu-jesse-stewart - any opinions on the best path forward here?
There was a problem hiding this comment.
In my opinion, if we can have same configs between tests and real env will be great to avoid future discrepancies between envs
There was a problem hiding this comment.
Ok, so I will keep it to have same config for build and tests.
There was a problem hiding this comment.
In my opinion, if we can have same configs between tests and real env will be great to avoid future discrepancies between envs
I 100% agree.
So this leaves us with:
| Option | Outcome |
|---|---|
Only change @babel/preset-react runtime for tests |
Builds might break in a way we don't catch because of the difference |
Change @babel/preset-react runtime for everything |
Possibly impact consuming applications? |
If this breaks tests for consuming MFEs then it should land on next as a breaking change, or we should find a way to do it as non-breaking. If we can confirm changing the @babel/preset-react runtime doesn't break anything for consuming MFEs then I think we can land it as non-breaking and add links to documentation about the implications of the change to the commit so it appears in the release notes.
There was a problem hiding this comment.
Actually it is only needed the one for the test env.
Considering that I found issues with React imports on the documentation site (see my comment: #3840 (review)
), the test errors we see without adding "runtime": "automatic" for @babel/preset-react actually reveal a real problem that should be addressed.
In my opinion, if we can have same configs between tests and real env will be great to avoid future discrepancies between envs
We definitely need to have consistent configurations across environments. That way, we can be confident that our tests accurately ensure the code works as expected 💯
There was a problem hiding this comment.
The config was changed to have the same on both environments.
jacobo-dominguez-wgu
requested review from
brian-smith-tcril and
diana-villalvazo-wgu
After researching I realized there was needed a special config on the gatsby-node for the documentation site in order to make the runtime automatic work, I added that and now all the components you mentioned were having issues now work ok. |
|
It appears that removing React imports from some components has caused issues in the Playground. The problem is triggered by the Button component, even though it (like the ActionRow component) no longer includes a direct React import. 
|
@PKulkoRaccoonGang I have pushed a new commit to solve this problem. |
babel.config.json
Outdated
| "presets": [ | ||
| ["@babel/preset-env", {}], | ||
| ["@babel/preset-react", { "useSpread": true } ], | ||
| ["@babel/preset-react", { "useSpread": true, "runtime": "automatic" } ], |
There was a problem hiding this comment.
we can keep same babel configs for test and build, so it can represent a non-breaking change.
@jacobo-dominguez-wgu if we’ve decided that the @babel/preset-react configuration should be used across all environments, and we’ve already added it to the presets on line 4 of this file, do we still need to duplicate @babel/preset-react under the test environment?
Since the presets section on line 4 defines the base configuration - for example, @babel/preset-env is defined there and then overridden with {} (CommonJS) for the test environment - it seems we can safely remove @babel/preset-react from env.test.presets and keep it only in the global configuration.
There was a problem hiding this comment.
You are right, I removed it from the test env config. The tests remain working.
|
@jacobo-dominguez-wgu I'm hoping to get some movement on this PR and the 10 other react import cleanup PRs. I think all the review comments on here have been addressed but I'd need to double check. @wgu-jesse-stewart do you have any bandwidth to look at this (and hopefully also the other 10)? For this one it'd just be great to get a fresh set of eyes on it to make sure the concerns raised in previous reviews are addressed and nothing else sticks out as problematic. Seeing a 👍 from you before I dive into another review on this would be wonderful! |
Thanks @brian-smith-tcril I am expecting to get this approved to apply the same changes to the other 10PRs. |
5 participants
Description
This PR replaces #3808, closed due to commit pollution and rebase issues.
Removing React imports that are not required anymore on newer versions. Some of the imports were deconstructed to remove the usage of the global React object.
Some files needs the global React import to handle JSX outside functions, if removed the build-docs command fails.
Most of the modifications were performed using the react/update-react-imports codemod.
Also modified the babel config file to use the JSX Transform (introduced in React 17).
Partially closes #3796
Components included:
Deploy Preview
Include a direct link to your changes in this PR's deploy preview here (e.g., a specific component page).
Merge Checklist
exampleapp?Post-merge Checklist