-
-
Notifications
You must be signed in to change notification settings - Fork 198
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
Add tests for CSS Modules with React and Preact #1343
Conversation
0f7130e
to
8f3aef2
Compare
babelConfig.presets.push([require.resolve('@babel/preset-react'), { | ||
// TODO: To remove when Babel 8, "automatic" will become the default value | ||
runtime: 'automatic', | ||
}]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick-win, let's bump @babel/preset-react
to ^7.9.0 to use runtime: 'automatic'
8f3aef2
to
a4450ee
Compare
@@ -48,7 +48,7 @@ | |||
"@babel/eslint-parser": "^7.17.0", | |||
"@babel/plugin-transform-react-jsx": "^7.12.11", | |||
"@babel/preset-env": "^7.16.0", | |||
"@babel/preset-react": "^7.0.0", | |||
"@babel/preset-react": "^7.9.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bumping the dev dependency alone is not enough. For our users, what matters for the package managers is the peerDependencies
section.
btw, we should update our features
utility to make it enforce versions based on peerDependencies
rather than devDependencies
now that we properly define peer dependencies. Or we should drop that runtime check entirely in favor of letting yarn/npm/pnpm show the warning at installation time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, forgot about peerDeps, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, we should update our
features
utility to make it enforce versions based onpeerDependencies
rather thandevDependencies
now that we properly define peer dependencies. Or we should drop that runtime check entirely in favor of letting yarn/npm/pnpm show the warning at installation time.
Totally! Maybe we can do 1) for v5, and 2) for v6?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the first change can even be done in a minor version (as that's about changing the internal implementation only). Our peerDependencies is already what package managers will report.
a4450ee
to
c22943b
Compare
Quick PR to add functional tests about using CSS Modules with React and Preact, to see it it also break CSS Modules with them in #1319.