mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.15k stars 1.29k forks source link

[DataGrid] ReferenceError: process is not defined #836

Closed matandro closed 3 years ago

matandro commented 3 years ago

I could not reproduce the error with codesandbox even with the same package version. The same code runs smooth on alpha.8. I am not sure what is the exact version where it breaks. Code is built using webpack with babel.

I receive an index-esm.js?:formatted:2044 Uncaught ReferenceError: process is not defined Error that is targeted to node_modules/@material-ui/x-grid/dist/index-esm.js. The error line is:`

On = void 0 !== process.env.EXPERIMENTAL_ENABLED && _r() && window.localStorage.getItem("EXPERIMENTAL_ENABLED") ? "true" === window.localStorage.getItem("EXPERIMENTAL_ENABLED") : "true" === process.env.EXPERIMENTAL_ENABLED;

Relevant packges list:

"@material-ui/core": "^4.11.2",
"@material-ui/icons": "^4.11.2",
"@material-ui/lab": "^4.0.0-alpha.57",
"@material-ui/x-grid": "^4.0.0-alpha.15",
"react": "^17.0.1",
"react-dom": "^17.0.1",

EDIT: I checked each version and the error pops up in alpha 11 (up to alpha 10 I don't see the error)

oliviertassinari commented 3 years ago

@matandro Thanks for the report. I suspect that your bundling environment is only configured to handle process.env.NODE_ENV correctly. Do you have a reproduction? It would greatly help us understand what's going on. Once we have a reproduction, we could test this potential solution:

diff --git a/packages/grid/_modules_/grid/constants/envConstants.ts b/packages/grid/_modules_/grid/constants/envConstants.ts
index b15fb77..165492d 100644
--- a/packages/grid/_modules_/grid/constants/envConstants.ts
+++ b/packages/grid/_modules_/grid/constants/envConstants.ts
@@ -18,15 +18,16 @@ import { localStorageAvailable } from '../utils/utils';
 // Developers (users) are discouraged to enable the experimental feature by setting the EXPERIMENTAL_ENABLED env.
 // Instead, prefer exposing experimental APIs, for instance, a prop or a new `unstable_` module.

-let experimentalEnabled;
+let experimentalEnabled = false;

 if (
+  typeof process !== 'undefined' &&
   process.env.EXPERIMENTAL_ENABLED !== undefined &&
   localStorageAvailable() &&
   window.localStorage.getItem('EXPERIMENTAL_ENABLED')
 ) {
   experimentalEnabled = window.localStorage.getItem('EXPERIMENTAL_ENABLED') === 'true';
-} else {
+} else if (typeof process !== 'undefined') {
   experimentalEnabled = process.env.EXPERIMENTAL_ENABLED === 'true';
 }
matandro commented 3 years ago

I will try to implement a small test case. For now I was reading about this issue (seems to be common). Webpack 5 no longer adds the process.env variable, to overcome the issue you can add a plugin to force it's creation:

  1. install npm process
  2. To webpack.config.js add
    
    const webpack = require('webpack');

module.export = { plugins: [ new webpack.ProvidePlugin({ process: 'process/browser', }), ], // ... rest of the module }



See more at https://webpack.js.org/migrate/5/
In general it seems the suggestion is not to use process at all, not sure why this change was made.
oliviertassinari commented 3 years ago

@matandro Awesome great, it seems to confirm that the proposed diff would cover your problem.

leontastic commented 3 years ago

I have this issue too. I'm using esbuild for bundling.

Even when I define process.env.EXPERIMENTAL_ENABLED in my bundler, I still get this runtime error. I can run process.env.EXPERIMENTAL_ENABLED and get true at runtime in my own bundle output, but when I require @material-ui/data-grid this error still occurs at runtime.

Looking at my build output I see the following two references (the only references in the whole project!) to process.env:

BI=(process&&process.env&&"true")!==void 0&&uP()&&window.localStorage.getItem("EXPERIMENTAL_ENABLED")?window.localStorage.getItem("EXPERIMENTAL_ENABLED")==="true":(process&&process.env&&"true")==="true";

I'm assuming this is because the index-esm.js build output from this library actually includes the process && process.env. From index-esm.js:

ta=void 0!==(process && process.env && process.env.EXPERIMENTAL_ENABLED)&&Wt()&&window.localStorage.getItem("EXPERIMENTAL_ENABLED")?"true"===window.localStorage.getItem("EXPERIMENTAL_ENABLED"):"true"===(process && process.env && process.env.EXPERIMENTAL_ENABLED);

The bundler you are using to produce index-esm.js seems to replace process.env.EXPERIMENTAL_ENABLED !== undefined with 0!==(process && process.env && process.env.EXPERIMENTAL_ENABLED) in the final build output.

Hence, users' bundlers replace process.env.EXPERIMENTAL_ENABLED with "true" but leave process and process.env untouched. This means process must actually exist in the browser runtime for users to run the bundled output without encountering the ReferenceError at runtime. This is why the ProvidePlugin must be used to provide a process object with Webpack builds, and the ReferenceError still happens using esbuild.

This doesn't seem to happen with NODE_ENV. All references to process.env.NODE_ENV === "production" are replaced with "production"!==process.env.NODE_ENV in the final index-esm.js output, which is replaced with "production"!=="development" or evaluated to true/false by users' bundlers. I'm guessing the comparison process.env.EXPERIMENTAL_ENABLED !== undefined could probably be rewritten in a way that your bundler doesn't split up process.env.EXPERIMENTAL_ENABLED into process && process.env && process.env.EXPERIMENTAL_ENABLED to avoid the issue, but I honestly don't know how it should be written.

I don't think @oliviertassinari's suggested diff will fix the issue because the process.env.EXPERIMENTAL_ENABLED !== undefined expression will probably still build into process && process.env && process.env.EXPERIMENTAL_ENABLED even if you add typeof process !== "undefined" && before it.

After doing so much research into this issue, I agree with @matandro it would probably be better to do away with this flag in general, and hide experimental features behind another version tag instead. That way, developers who want experimental features can get them simply by checking out a different branch or version, and you can avoid issues related to bundlers' syntax transform rules entirely. Or if you can figure out a way to rewrite process.env.EXPERIMENTAL_ENABLED !== undefined without the bundler splitting the expression up, maybe that would be a sufficient solution.

Either way, this bug makes the library completely unusable for me since I don't have a readily-available Provider plugin for esbuild to polyfill process, and frankly speaking I don't really want to add hacks to my code just to accomodate this experimental feature flag. Will just check out a pre-alpha version instead.

@oliviertassinari Bless the work you did, this library is still pretty great.

oliviertassinari commented 3 years ago

@leontastic Thanks for looking into it.

Under https://unpkg.com/@material-ui/data-grid@4.0.0-alpha.19/dist/index-esm.js I see:

let ta;
ta = void 0 !== process.env.EXPERIMENTAL_ENABLED && Wt() &&
  window.localStorage.getItem("EXPERIMENTAL_ENABLED")
    ? "true" === window.localStorage.getItem("EXPERIMENTAL_ENABLED")
    : "true" === process.env.EXPERIMENTAL_ENABLED;

I'm pretty sure that the proposed fix would work. However, we could try it out with a pull request, and test it instantly with codesandbox-ci before merging. Do you want to give it a try? :)

lnorton89 commented 3 years ago

I've tried every workaround I've found--using ProvidePlugin and other ways to make process available via my Webpack config-- and cannot get this error to stop.

I checked each version and the error pops up in alpha 11 (up to alpha 10 I don't see the error)

This also does not seem to be true for me. I've tried every Alpha version down to 9 and they still error out. Is there a reason we're going against Webpacks own advice not to use process?

process.env is Node.js specific and should be avoided in frontend code.

leontastic commented 3 years ago

@oliviertassinari I put your diff into a PR so you can try: https://github.com/mui-org/material-ui-x/pull/1027

BUT I think your fix would break the feature flag itself.

Even if process.env.EXPERIMENTAL_ENABLED is defined by users' bundler, if we assume process remains undefined then typeof process !== 'undefined' still evaluates to false, so both of the code paths in envConstants.ts will be skipped, hence experimentalEnabled will always remain false. Might as well just export EXPERIMENTAL_ENABLED = false in that case.

A full-text search of the entire project reveals that this flag is not even being used! Personally I would remove this flag. If that's not an option, I would set it to a string so that the bundler doesn't split up the process.env.EXPERIMENTAL_ENABLED !== undefined check into process && process.env .... I think that's why checks against process.env.NODE_ENV === 'production' don't seem to result in the issue we are seeing here.

leontastic commented 3 years ago

Alternative PR removing this file (should cause 0 difference since the flag isn't being used anyway): #1028

Consider merging this PR in and re-adding an experimental flag when you actually introduce experimental features to hide. This way you can test both the experimental feature and the correctness of this flag end-to-end instead of needing to reason with the code.

leontastic commented 3 years ago

@oliviertassinari I tested both PRs from codesandbox and they both work – No more ReferenceError. I also did a text-search check for process.env.EXPERIMENTAL_ENABLED and it looks like your fix actually did prevent it from getting split up into process && process.env && ... in the bundle output.

I don't know if the actual flag will work though because typeof process !== 'undefined' might return false even if process.env.EXPERIMENTAL_ENABLED is defined by my bundler. In this case the EXPERIMENTAL_ENABLED constant would always be false no matter what I set it to.

Which PR you merge is up to you! Thanks for your hard work.

oliviertassinari commented 3 years ago

@leontastic perfect 👌