cljsjs / packages

DEPRECATED: Javascript libraries packaged up with Google Closure externs
http://cljsjs.github.io
786 stars 816 forks source link

[react-popper] Uncaught ReferenceError: process is not defined #1579

Open nijk opened 6 years ago

nijk commented 6 years ago

When attempting to use react-popper with Closure Compiler's :optimizations :advanced I get a javascript error that process is not defined. I believe this is because ReactPopper uses the transform-react-remove-prop-types babel plugin which wraps the PropTypes in a conditional referencing process.env.NODE_ENV to allow for Dead Code Elimination (DCE). This doesn't seem to be appropriate for a production build as the Dead Code is not actually eliminated from the minified asset and it relies on a global which is not normally available in production React environments.

This may be an issue for the ReactPopper dependency to resolve, but I wanted to highlight it for others here and get an understanding of the options for resolving this.

@EmergentBehavior I noticed that you recently updated the react-popper package, did you experience this issue locally? Any tips or suggestions would be greatly appreciated

EmergentBehavior commented 6 years ago

I did run into this issue as well, unfortunately. My short term solution was to define process ahead of the loading of my compiled application JS:

<script>var process = {env: {}};</script>

Additionally, you could define NODE_ENV within if you desired.

Longer term solution might be to (a) notify the developer or (b) create a new patch release of react-popper here that builds without this.

nijk commented 6 years ago

Ok, well I'm feeling a little more vindicated in my struggles with this particular problem as I came to the same conclusion.

I think there is one more solution and that is to create a webpack config in the cljsjs package that builds react-popper from source without the transform-react-remove-prop-types babel plugin. The build function could then shell out to npm install using the --ignore-scripts flag and call its own build process using the webpack config.

I wonder if a) is the right solution with c) being the temporary stop-gap. I am slightly concerned that as ReactPopper has moved onto 1.x releases, change requests to 0.x may not be a priority. That said, the maintainer is very responsive to GH issues

nijk commented 6 years ago

@EmergentBehavior Out of interest, do you know if it is possible to dynamically define process.env.NODE_ENV with :optimizations :advanced - myself and a colleague tried multiple ways to get it working but we couldn't get process to be defined as a global even when we supplied process as an extern.

The use case here is about: working around this issue and having the PropType warnings behave correctly in both development & production builds.

EmergentBehavior commented 6 years ago

I think there is one more solution and that is to create a webpack config in the cljsjs package that builds react-popper from source without the transform-react-remove-prop-types babel plugin. The build function could then shell out to npm install using the --ignore-scripts flag and call its own build process using the webpack config.

I agree. Might be worth doing this and then opening an issue with https://github.com/FezVrasta/react-popper to nudge them to fix it.

Out of interest, do you know if it is possible to dynamically define process.env.NODE_ENV with :optimizations :advanced - myself and a colleague tried multiple ways to get it working but we couldn't get process to be defined as a global even when we supplied process as an extern.

For our application, we dynamically generate the index.html files depending on environment, so you could create a template and then selectively include the process definition if it's a production build.

You could also try selectively running (if prod build) something like (set! js/process {}) as long as it's defined before the popper dependency is loaded.

There might be a more efficient way but I can't think of it off top of my head.

nijk commented 6 years ago

Thanks @EmergentBehavior, I appreciate your input and especially the quick response.

I'm planning to raise a webpack build PR on react-popper in the next few days, then I'll raise an issue on their repo.

EmergentBehavior commented 6 years ago

@nijk I'm also getting an intermittent error around Uncaught TypeError: Cannot read property 'placements' of undefined -- are you?

EmergentBehavior commented 6 years ago

I think because they call (in unminifed file) var placements = PopperJS.placements; and PopperJS is undefined.

EmergentBehavior commented 6 years ago

The issue was a corrupted PopperJS build (because of the bundled popperutils), see https://github.com/cljsjs/packages/pull/1582 and https://github.com/cljsjs/packages/pull/1583

nijk commented 6 years ago

Sorry for the delay on this, I've gone down a bit of a rabbit hole with this issue in ReactPopper which has resulted in finding an upstream issue in the babel-plugin-transform-react-remove-prop-types lib - this lib doesn't seem to be actively maintained as my issue has gone for 5 days without reply so I'm going to pursue this approach in the cljsjs package for ReactPopper:

Create a webpack config in the cljsjs package that builds react-popper from source without the transform-react-remove-prop-types babel plugin. The build function could then shell out to npm install using the --ignore-scripts flag and call its own build process using the webpack config.