@neutrinojs/react-components expects prop-types to be available in build

lb- commented 5 years ago

Bug

What version of Neutrino are you using? 8.3.0
Are you trying to use any presets? If so, which ones, and what versions?
Are you using the Yarn client or the npm client? What version?
- "@neutrinojs/airbnb": "^8.3.0",
- "@neutrinojs/jest": "^8.3.0",
- "@neutrinojs/react-components": "^8.3.0",
What version of Node.js are you using? v10.15.3
What operating system are you using? macOS

What did you do?

yarn create @neutrinojs/project client
First up, what would you like to create? Components
Next, what kind of components would you like to create? React Components
Would you like to add a test runner to your project? Jest
Would you like to add linting to your project? Airbnb style rules
cd ./client
npm run build
Add the build js output to a script tag to a html file <script src="/static/HelloWorld.js"></script>
The html file is a template in a Django (Python) project, which already has react in the global available via window.React

add the following HTML

<script>
document.addEventListener('DOMContentLoaded', function() {
  const component = HelloWorld.default;
  ReactDOM.render(React.createElement(component, { className: 'content' }), document.getElementById('root'));
});
</script>
<body><div id="root"></div></body>

Run the project

What did you expect to happen?

The HelloWorld react component should mount on the #root div
What actually happened, contrary to your expectations?

require is not defined see #1425 - this issue is NOT about the require being needed.
After I 'mocked' a basic require function to essentially return React from the global namespace, then it said 'prop-types' is not defined.

I expected that prop-types are stripped from the build because of this code, which react-components uses: https://github.com/neutrinojs/neutrino/blob/master/packages/react/index.js#L28

Instead, I think what is happening is that that the webpack-node-externals is somehow thinking that the project needs prop-types because of its default behaviour call when used here:

https://github.com/neutrinojs/neutrino/blob/master/packages/react-components/index.js#L63

I think the actual prop-types are being stripped, but maybe that happens AFTER the require calls are added in the build process. I do not really know enough about webpack to know for sure but if I skip that step and define my own externals via a custom preset at the end, prop-types is not required.

Work around + potential source of the problem

This code below solves the 'require' problem but as an aside, overriding the output in config.externals that is set by webpack-node-externals seems to resolve the issue of prop-types being expected to be available.

eg.

module.exports = {
  use: [
    "@neutrinojs/airbnb",
    "@neutrinojs/react-components",
    "@neutrinojs/jest",
    /** ensure that react is read from global - and webpack-node-externals is NOT used */
    neutrino => {
      neutrino.config.when(process.env.NODE_ENV === "production", config => {
        config.externals({ react: "React" });
      });
    }
  ]
};

edmorley commented 5 years ago

Hi! Sorry for the delayed reply, and thank you for filing this issue.

I agree that prop-types should not be required in production. The code on master linked above is for Neutrino 9, whereas Neutrino 8 didn't use babel-plugin-transform-react-remove-prop-types - see the v8 release branch: https://github.com/neutrinojs/neutrino/blob/release/v8/packages/react/index.js

ie: Stripping prop-types is a new feature in Neutrino 9, which is currently in release candidate (see #1129) and I'd recommend using it over Neutrino 8 even before the final release :-)

lb- commented 5 years ago

@edmorley - epic news! thank you.

I am doing a blog post about using Neutrino and will try to upgrade my code to use v9 locally to see if it all works as expected.

neutrinojs / neutrino