Skip to content
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

[CDN] Fix the UMD build #13928

Merged
merged 1 commit into from
Dec 18, 2018
Merged

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Dec 17, 2018

Uses the same solution as reduxjs/react-redux#1062. You can reproduce the error following this link: https://rawgit.com/mui-org/material-ui/master/examples/cdn/index.html. I believe that we should have some kind of test for the umd build!

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work priority: important This change can make a difference labels Dec 17, 2018
@@ -21,6 +21,9 @@ const babelOptions = {
const commonjsOptions = {
ignoreGlobal: true,
include: /node_modules/,
namedExports: {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eps1lon
Copy link
Member

eps1lon commented Dec 18, 2018

I don't think that this was the root cause of the issue since hoist-non-react-static used react-is the same way (mridgway/hoist-non-react-statics#72). This was probably only because we used require instead of import. We should add a lint rule for that.

I believe that we should have some kind of test for the umd build!

Maybe adjust our test to import straight from @material-ui/core and let babel alias resolve this differently depending on the requirement. From source for PRs and from (cjs|umd|esm)-build on prerelease. Would greatly improve the confidence in our build pipeline.

@oliviertassinari
Copy link
Member Author

This was probably only because we used require instead of import. We should add a lint rule for that.

@eps1lon It doesn't "work" either. Using an import raises a warning at build and runtime. At some point, I have tried using a require to make the roll-up warning went away. Now, I know that rollup can fail to build the source correctly without raising. It's not what my experience with webpack has been so far.

@oliviertassinari oliviertassinari merged commit 5773bd0 into mui:master Dec 18, 2018
@oliviertassinari oliviertassinari deleted the fix-cdn-live branch December 18, 2018 20:10
@eps1lon
Copy link
Member

eps1lon commented Dec 18, 2018

A warning at runtime? From rollup?

Technically a named import isn't allowed at the moment anyway until react-is has a module build. So this is something that needs to be revisited once facebook/react#13321 is released.

@oliviertassinari
Copy link
Member Author

@eps1lon It's good to know. I think that we can keep up with react-redux build system and improve our CI.

@oliviertassinari oliviertassinari mentioned this pull request Dec 20, 2018
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work priority: important This change can make a difference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants