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

[ButtonBase] Fix process is not defined #13252

Merged
merged 3 commits into from
Oct 15, 2018

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Oct 15, 2018

Closes #13247

I tried to not mock process but we execute on dependencies that are built for node (e.g. enzyme) so not polyfilling this will cause all sorts of problems in transitive dependencies.

A better place for removing the mock might be our visual regression tests.

Copy link

@KevinGrandon KevinGrandon left a comment

Choose a reason for hiding this comment

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

I think this should work, thank you very much!

@oliviertassinari oliviertassinari merged commit 3a474de into mui:master Oct 15, 2018
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: button This is the name of the generic UI component, not the React module! labels Oct 15, 2018
@oliviertassinari
Copy link
Member

It would be great to have some sort of regression tests. The best story for this problem so far is that the only usage of process that is OK is process.env.NODE_ENV.

@KevinGrandon
Copy link

It would be great to have some sort of regression tests. The best story for this problem so far is that the only usage of process that is OK is process.env.NODE_ENV.

Actually I don't even think using that would be ok if you use MUI outside of webpack (unless you explicitly guard it with a typeof window === "undefined" && process type of check.

For the regression test, I assume we would need new infrastructure that does not use the existing webpack config. (Maybe just a html file with a link to the built bundle or something).

@eps1lon
Copy link
Member Author

eps1lon commented Oct 15, 2018

@KevinGrandon You have to convince the ecosystem first. Even react uses process.env.NODE_ENV in their build output.

Ideally we would run our regression tests with node: false but I can't get them to run locally (hangs) and the one build without polyfilled node returned a unhelpfull error message: https://circleci.com/gh/mui-org/material-ui/50343

@eps1lon eps1lon deleted the fix/process-not-defined branch October 15, 2018 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: button This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants