Skip to content
This repository has been archived by the owner on Dec 31, 2020. It is now read-only.

Reference error: process is not defined. #801

Closed
ynejati opened this issue Nov 14, 2019 · 15 comments
Closed

Reference error: process is not defined. #801

ynejati opened this issue Nov 14, 2019 · 15 comments

Comments

@ynejati
Copy link
Contributor

ynejati commented Nov 14, 2019

Line 14 in Provider.js. If the global 'process' is not defined, it can cause the browser to throw an error. This seems like an oversight.

image-1

@danielkcz
Copy link
Contributor

Definitely not an oversight, more like an issue of bundling tool - microbundle: developit/microbundle#115

@ynejati
Copy link
Contributor Author

ynejati commented Nov 14, 2019

I would disagree. Sure, you could configure your bundler to add the global, but that shouldn't be a requirement. What if someone is using a CDN in a script tag. Are you going to require that they also add <script>process = { env : null}</script>? That doesn't makes sense.

@danielkcz
Copy link
Contributor

danielkcz commented Nov 14, 2019

No, I mean that microbundle is used for building this package, but fails at removing process checks. It might be more fruitful to discuss this at that linked issue.

Personally, I prefer TSDX tool which has this solved, but that would require rewriting the lib to TypeScript. Something I want to do for a long time, but still, lack the time :)

@ynejati
Copy link
Contributor Author

ynejati commented Nov 15, 2019

I misinterpreted your response. Apologies. That makes sense, since typically this logic is/should be removed during minification/creation of the production bundle. Nonetheless, this has broken our production build, when upgrading to the latest release. Currently the work around is to ensure process.env exists in the global scope, which is less than ideal.

I've never heard of TSDX, looks interesting. Thanks for sharing.

Yes, rewriting the lib to TypeScript would take some time. It would certainly be consistent with mobx and feel more uniform, to say the least. If you think you'd like help with this, let me know.

@danielkcz
Copy link
Contributor

@ynejati

Yes, rewriting the lib to TypeScript would take some time. It would certainly be consistent with mobx and feel more uniform, to say the least. If you think you'd like help with this, let me know.

I don't think it would be too difficult. Since V6, the package is fairly trimmed down with most of the core logic in mobx-react-lite and general simplification with Hooks. If you are feeling for it, PR is definitely most welcome.

Nonetheless, this has broken our production build, when upgrading to the latest release.

That's strange considering that line of code was there more or less half a year. I guess you can stay on the older version for now till this is properly figured out.

@mweststrate
Copy link
Member

mweststrate commented Nov 15, 2019 via email

@mweststrate
Copy link
Member

mweststrate commented Nov 15, 2019 via email

@ynejati
Copy link
Contributor Author

ynejati commented Nov 15, 2019

Seems that mobx-react-typescript-boilerplate's mobx-react.module.js file looks like this, properly bundling and aliasing to true because it actually runs through webpack:
image

For us, we use mobx-react as an external, grabbing the "production bundle" from node_modules and injecting into a script tag. We did the same before upgrading from 5 to 6. Ideally we would use Webpack's expose-loader, to do this, but the plugin appears to be broken for webpack 4. As a result, since mobx-react.module.js still contains process.env it breaks our build.

@ynejati
Copy link
Contributor Author

ynejati commented Nov 15, 2019

Yes, I agree. It would be nice to add a simple check like process && process.env similar to the mobx minified build. Might have to do this for mobx-react-lite as well.

@danielkcz
Copy link
Contributor

danielkcz commented Nov 15, 2019

Well, the mobx-react.module.js should definitely stay as close as possible to the original code. ESM format is something that bundlers need to handle and it shouldn't be used out of the bundling system. Had the annoying issue in the past which basically lead to TSDX leaving ESM format as is. The CJS and UMD formats are surely a different story.

@mweststrate

typically a typeof process !== 'undefined' as prefix for the condition fixes this.

I would argue this is a scalable solution. How annoying would that be with several dev-only checks in the code? I know it does not apply to this case but feels like starting some weird trend instead of fixing the issue at its root 🤷‍♂

What I also love about TSDX that it will expose __DEV__ global variable and then you don't need to worry about the lengthy check and it will be correctly transformed. Not sure why microbundle is so badly lacking behind.

@mweststrate
Copy link
Member

mweststrate commented Nov 15, 2019 via email

@ynejati
Copy link
Contributor Author

ynejati commented Nov 15, 2019

@mweststrate Thanks for the heads up. We are actually using the UMD build already, but good to know.

@FredyC You sure are pushing TSDX pretty hard. I'm beginning to think that they are paying you. ;)

@ynejati
Copy link
Contributor Author

ynejati commented Nov 16, 2019

In mobx's entry point, mobx.ts, we conveniently have:

try {
    // define process.env if needed
    // if this is not a production build in the first place
    // (in which case the expression below would be substituted with 'production')
    process.env.NODE_ENV
} catch (e) {
    const g = getGlobal()
    if (typeof process === "undefined") g.process = {}
    g.process.env = {}
}

Should we do the same here?

Curious though, why mobx being a peer dependency and supposedly loaded first on the page didn't prevent my problem?

mweststrate added a commit that referenced this issue Jan 12, 2020
Issue #801: Check if process.env is defined when using Provider
@mweststrate
Copy link
Member

Released in 6.1.5

@lock
Copy link

lock bot commented Mar 17, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions.

@lock lock bot locked as resolved and limited conversation to collaborators Mar 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants