-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[icons] Only ship ES modules #26310
[icons] Only ship ES modules #26310
Conversation
c57cc79
to
7e6b6c8
Compare
7e6b6c8
to
d920e73
Compare
d920e73
to
c3f24cb
Compare
The package built by codesandbox-ci works with CRA. The size of the archive goes from 1.7 MB to 1.2 MB gzipped, and the uncompressed size from 15.6 MB to 7.2 MB. This looks awesome 🚀! |
@eps1lon Is there any blocker to push this PR further? What do you think about releasing the changes and wait for the community to try it out to gather the limitations? |
Our users are not Guinea pig |
@eps1lon I meant for the alpha release channel, not the stable one. It seemed to be effective and efficient and with little downsides, but OK. As long as we are no overcareful/perfectionist, fair enough 👍 . |
b2045d2
to
3fa2921
Compare
3fa2921
to
c3f374d
Compare
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.
It looks great 🚀, two questions:
publishConfig.exports
andexports
have the same value in the package.json. Why do we need the duplication?- We usually don't mention the
.js
extension in the imports. Why is an implicit extension not enough?
They should not have the same value. This is what I'm seeing (old: exports, new: publishConfig.exports) when checking out the latest state: "exports": {
- ".": "./lib/index.js",
+ ".": "./index.js",
- "./*": "./lib/*.js",
+ "./*": "./*.js",
"./utils/*": null
},
Because ES modules need the full file path. |
OK, it's not a direct duplication, you are right. Would it make sense automatically rewrite the
Oh wow, I didn't know that! |
Maybe it is. We'll see. There's no value in thinking about abstraction with this little information. |
@oliviertassinari I have reverted your change to the PR description |
The new description: "The |
Upgrading to alpha 36 of @material-ui/icons seems to breaks Next.js apps cause it doesn't support ESM natively. To get around, I had to either dynamically import each icon and disable ssr or transpile as referenced in vercel/next.js#25423 (comment) |
Thanks for the report. We've accidentally set up our integration tests incorrectly and didn't catch that. We'll revert it for now (probably 1 week till release) until we come up with a fix. |
This reverts commit a6f79d4.
@jerryhuang3 How do you reproduce? diff --git a/examples/nextjs/src/ProTip.js b/examples/nextjs/src/ProTip.js
index 3d7e09703b..1eedf12ff8 100644
--- a/examples/nextjs/src/ProTip.js
+++ b/examples/nextjs/src/ProTip.js
@@ -1,15 +1,7 @@
import * as React from 'react';
import Link from '@material-ui/core/Link';
-import SvgIcon from '@material-ui/core/SvgIcon';
import Typography from '@material-ui/core/Typography';
-
-function LightBulbIcon(props) {
- return (
- <SvgIcon {...props}>
- <path d="M9 21c0 .55.45 1 1 1h4c.55 0 1-.45 1-1v-1H9v1zm3-19C8.14 2 5 5.14 5 9c0 2.38 1.19 4.47 3 5.74V17c0 .55.45 1 1 1h6c.55 0 1-.45 1-1v-2.26c1.81-1.27 3-3.36 3-5.74 0-3.86-3.14-7-7-7zm2.85 11.1l-.85.6V16h-4v-2.3l-.85-.6C7.8 12.16 7 10.63 7 9c0-2.76 2.24-5 5-5s5 2.24 5 5c0 1.63-.8 3.16-2.15 4.1z" />
- </SvgIcon>
- );
-}
+import LightBulbIcon from '@material-ui/icons/LightBulb';
export default function ProTip() {
return ( |
This reverts commit a6f79d4.
@material-ui/icons
no longer ships with CommonJS modules. We now ship standard compliant ES6 modules.This reduces install size from package size from 15.6 MB to 6.7 MB and packed size (relevant when fetching the package from package registries causing #26240) from 1.7 MB to 1.2 MB. Note that this only affects the size relevant to development. The bundle shipped to your users should should have negligible size differences.
BREAKING CHANGE
The
require()
of@material-ui/icons
is no longer supported. This should not affect you if you're using a bundler likewebpack
orsnowpack
or meta frameworks likenext
orgatsby
.Closes #26240