Skip to content

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 type: bug It doesn't behave as expected. priority: important This change can make a difference. labels Dec 17, 2018
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

priority: important This change can make a difference. type: bug It doesn't behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants