v0.10.1: Consider removing transform-react-remove-prop-types mode of 'wrap'

[nijk]

Reproduction demo

Firstly I'd like to thank you for this library and Popper.js.

I'm currently trying to get this working in a ClojureScript app and I'm having some real difficulties getting Closure Compiler not to strip the process.env variable in it's advanced optimization mode. It's a bit of a pain but I've managed to isolate the need for the process var down to this dependency. As far as I can tell the wrap mode is not the default configuration for this reason:

The wrap modes are targeting React libraries like material-ui or react-native-web. They are not intended to be used by application authors.

• plugin docs

Steps to reproduce the problem

1. Install this library and import/require it into a React project
2. Build the project in production mode and access it in a browser
3. See that the browser throws an unhandled exception of process not defined unless process is explicitly provided (as is done in the v0.x CodePen template)

What is the expected behavior?

Ideally not to have a dependency on window.process.env in either development or production builds

What went wrong?

I think this could easily be solved by removing , { mode: 'wrap' } from rollup.config.js.

Any other comments?

I'm happy to raise a PR for this if you would be happy to consider it.

Packages versions

• Popper.js: any
• react-popper: v.0.10.1

[FezVrasta]

0.10.1 is mostly maintenance only at this point. Most of the consumers prefer to have a good DX rather than the ability to run the unminified package in the browser.

I'd rather keep the things as they are, feel free to switch to the 1.x branch, it's stable at this point.

[nijk]

Only problem, is that process does exist within the minified version, e.g.

grep process dist/react-popper.umd.min.js => k.propTypes='production'===process.env.NODE_ENV?{}: ...

[FezVrasta]

Thanks for pointing it out, please feel free to send a PR to strip it out from the minified build if you have some time to allocate to it. Thanks! 🤩

[nijk]

Will do, thanks 👍

[nijk]

Ok, there's a bit of Yak shaving going on now. I've made the change but the babel plugin is not removing propTypes from the stateless functional components - I've raised this issue with babel-plugin-transform-react-remove-prop-types

[virgofx]

@nijk The process.env part should get removed completely. That's definitely a bug. Webpack has the DefinePlugin which handles that. Not sure how it's being built here but those comparisons should be removed from all production builds 100%.

The original intent for getting babel prop-types in the build was just to reduce any hard dependency on prop-types in the production distribution builds, which contrary to the statement above, is being used by a lot of our Reactstrap users (As we've specifically pointed to this in our Readme to use a light version of Reactstrap with optional dependencies).

Keep us posted what you find and thanks for helping get the builds sorted out.

[nijk]

Thanks @virgofx, it's a pleasure :)

This library uses rollup rather than webpack. My understanding is that the Webpack DefinePlugin allows the inclusion of global vars at compile time as opposed to the removal of references in the compiled code, as such process.env would exist in the code and the JS interpreter would not throw a undefined var error.

The issue at play here should be solved if the maintainers at babel-plugin-transform-react-remove-prop-types can provide a fix to the propTypes default behaviour of { mode: 'remove' } for the functional components. I've not heard anything from them yet, but once I do I'll either raise a PR that fixes the issue or post back here with reasons as to why it can't be fixed.

[virgofx]

Server side rendering is broken actually with the new UMD build

Manager.propTypes = process.env.NODE_ENV !== "production" ? {

Need to fix this.