Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[commonjs] bundle React after deprecated namedExports #423

Closed
popeindustries opened this issue May 28, 2020 · 18 comments
Closed

[commonjs] bundle React after deprecated namedExports #423

popeindustries opened this issue May 28, 2020 · 18 comments

Comments

@popeindustries
Copy link

  • Rollup Plugin Name: commonjs
  • Rollup Plugin Version: 12.0.0
  • Rollup Version: 2.11.2
  • Operating System (or Browser): MacOS 10.15
  • Node Version: 14.3.0

How Do We Reproduce?

https://repl.it/repls/RoundedSnoopyBackend

  1. create Rollup config with React input file: input: require.resolve('react'), and with @rollup/[email protected] and @rollup/[email protected] plugins
  2. bundle esm output file

Expected Behavior

All React named exports are exported.

Actual Behavior

Named exports are assigned to local variables but not exported:

var react_development_1 = react_development.Children;
var react_development_2 = react_development.Component;
var react_development_3 = react_development.Fragment;
var react_development_4 = react_development.Profiler;
var react_development_5 = react_development.PureComponent;
var react_development_6 = react_development.StrictMode;
var react_development_7 = react_development.Suspense;
var react_development_8 = react_development.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED;
var react_development_9 = react_development.cloneElement;
var react_development_10 = react_development.createContext;
var react_development_11 = react_development.createElement;
var react_development_12 = react_development.createFactory;
var react_development_13 = react_development.createRef;
var react_development_14 = react_development.forwardRef;
var react_development_15 = react_development.isValidElement;
var react_development_16 = react_development.lazy;
var react_development_17 = react_development.memo;
var react_development_18 = react_development.useCallback;
var react_development_19 = react_development.useContext;
var react_development_20 = react_development.useDebugValue;
var react_development_21 = react_development.useEffect;
var react_development_22 = react_development.useImperativeHandle;
var react_development_23 = react_development.useLayoutEffect;
var react_development_24 = react_development.useMemo;
var react_development_25 = react_development.useReducer;
var react_development_26 = react_development.useRef;
var react_development_27 = react_development.useState;
var react_development_28 = react_development.version;

var react = createCommonjsModule(function (module) {

if (process.env.NODE_ENV === 'production') {
  module.exports = react_production_min;
} else {
  module.exports = react_development;
}
});

export default react;
@simenbrekken
Copy link

simenbrekken commented May 28, 2020

The culprit seems to be an if-statement at the head of react.development.js that reads:

if (process.env.NODE_ENV !== "production") {

Inside of that isn an IIEF that modifies exports

Uncommenting the if-statement produces a working bundle:

https://repl.it/repls/OldAliveClass

@popeindustries
Copy link
Author

And adding @rollup/plugin-replace to inline process.env.NODE_ENV does not fix the issue

@lukastaegert
Copy link
Member

The unused variables are indeed rather useless but it looks to me that otherwise, react is correctly assigned to module.exports. So is this just about getting rid of the unused assignments? Or is something functionally not working?

@popeindustries
Copy link
Author

This is certainly a problem with the React package itself, but earlier versions of the commonjs plugin had an escape hatch with namedExports where you could manually add all the missing exports

@popeindustries
Copy link
Author

popeindustries commented May 28, 2020

@lukastaegert this is about being able to import named exports in addition to default. react-redux, for example, tries to import useMemo: import {useMemo} from 'react'.

In fact, the cjs source for React only exports named exports:

exports.Children = Children;
exports.Component = Component;
exports.Fragment = REACT_FRAGMENT_TYPE;
exports.Profiler = REACT_PROFILER_TYPE;
exports.PureComponent = PureComponent;
exports.StrictMode = REACT_STRICT_MODE_TYPE;
exports.Suspense = REACT_SUSPENSE_TYPE;
exports.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED = ReactSharedInternals;
exports.cloneElement = cloneElement$1;
exports.createContext = createContext;
exports.createElement = createElement$1;
exports.createFactory = createFactory;
exports.createRef = createRef;
exports.forwardRef = forwardRef;
exports.isValidElement = isValidElement;
exports.lazy = lazy;
exports.memo = memo;
exports.useCallback = useCallback;
exports.useContext = useContext;
exports.useDebugValue = useDebugValue;
exports.useEffect = useEffect;
exports.useImperativeHandle = useImperativeHandle;
exports.useLayoutEffect = useLayoutEffect;
exports.useMemo = useMemo;
exports.useReducer = useReducer;
exports.useRef = useRef;
exports.useState = useState;
exports.version = ReactVersion;

@popeindustries
Copy link
Author

...actually, that's not correct either. The entry file re-exports all named exports as default:

module.exports = require('./cjs/react.development.js');

So Rollup is correct here, but users of React expect and rely upon named exports, and without the namedExports option it's not possible to generate a stand-alone bundle of React that other packages can use.

@lukastaegert
Copy link
Member

Well, the problem is that for CommonJS, the concept of a named export is only an illusion: There only is an exports object, and assuming that properties of that object have a special meaning if I added them one-by-one instead of just setting the object in one go is merely a heuristic. So if the consumer of your package is again a Rollup build, there should not be a problem as the Rollup build can again pick all properties from the default export if it is using the commonjs plugin. So I assume the problem is with... other bundlers?

@popeindustries
Copy link
Author

The use case here is "next generation" dev environments like Snowpack or (my) dvlp that run cjs packages through Rollup to convert them to esm for import directly in the browser (without further bundling). So react is bundled separately from react-redux, which relies upon named exports (that don't exist, as you point out). With namedExports, we could monkey-patch those missing named exports manually.

@lukastaegert
Copy link
Member

Well, since it is an entry point you could also just bundle an ESM wrapper file that imports React and exports the relevant properties as actual named exports. I do not see how this is much more effort than manually maintaining a list of named exports in a config file and it would make things more explicit. Or of course bother the React time to switch to this hipster module format that was developed only five years ago. But another story for another time I guess...

@popeindustries
Copy link
Author

Ok, will try my luck with a custom wrapper.

@lukastaegert
Copy link
Member

I think something as simple as

export {Component, Fragment, useState, } from 'react' 

should work wonders and should provide minimal overhead.

@Akiyamka
Copy link

@lukastaegert, could you explain me please how stupid me can fix the problem error-name-is-not-exported-by-module without namedExports now?
Where exactly i must place export {Component, Fragment, useState, …} from 'react' and how to use it later?

@lukastaegert
Copy link
Member

If React is bundled dependency and you use latest @rollup/plugin-commonjs and rollup, then everything should work automatically due to the magic of synthetic named export interpolation.

The issue here is that people want to bundle React as an entry point that should have the correct exports. As automatic named export detection is not possible due to the way React is distributed, you would make the wrapper your entry point instead.

But to me it sounds this is not what you want to do.

@Akiyamka
Copy link

Akiyamka commented Aug 27, 2020

@lukastaegert you right, I have error-name-is-not-exported-by-module error with xferno package, using link from error I found in docs that namedExports can fix that, but I see that already deprecated
https://github.com/Akiyamka/rollup-xferno-import
chrisdavies/xferno#2

@SagnikPradhan
Copy link

SagnikPradhan commented Nov 3, 2020

@lukastaegert Been working on adding support for peer dependencies in my rollup wrapper. I do not see a way to force peer deps to use default export rather than named exports for react or any other similar package, any idea what should I be doing?

Jyrno42 added a commit to Jyrno42/tg-spa-utils that referenced this issue Mar 6, 2021
It is a small wrapper around tg-named-routes which allows one to
have language specific routes.

Also:

- Updated rollup and plugins to resolve issue with React named imports
  - see rollup/plugins#423 (comment)
- Updated react used in development
- Updated react-router/react-router-dom
Jyrno42 added a commit to Jyrno42/tg-spa-utils that referenced this issue Mar 6, 2021
It is a small wrapper around tg-named-routes which allows one to
have language specific routes.

Also:

- Updated rollup and plugins to resolve issue with React named imports
  - see rollup/plugins#423 (comment)
- Updated react used in development
- Updated react-router/react-router-dom
Jyrno42 added a commit to Jyrno42/tg-spa-utils that referenced this issue Mar 6, 2021
It is a small wrapper around tg-named-routes which allows one to
have language specific routes.

Also:

- Updated rollup and plugins to resolve issue with React named imports
  - see rollup/plugins#423 (comment)
- Updated react used in development
- Updated react-router/react-router-dom
Jyrno42 added a commit to Jyrno42/tg-spa-utils that referenced this issue Mar 6, 2021
It is a small wrapper around tg-named-routes which allows one to
have language specific routes.

Also:

- Updated rollup and plugins to resolve issue with React named imports
  - see rollup/plugins#423 (comment)
- Updated react used in development
- Updated react-router/react-router-dom
Jyrno42 added a commit to Jyrno42/tg-spa-utils that referenced this issue Mar 6, 2021
It is a small wrapper around tg-named-routes which allows one to
have language specific routes.

Also:

- Updated rollup and plugins to resolve issue with React named imports
  - see rollup/plugins#423 (comment)
- Updated react used in development
- Updated react-router/react-router-dom
@cosmikwolf
Copy link

I am curious if anyone has found a solution to getting react named exports to work with rollup? This issue was closed, but there still doesnt seem to be any solution

@JaffParker
Copy link

It's July 2023 and there still seems to be no solution. We deprecated the option that actually fixed this? I don't understand, there's no way no one uses Rollup with React and, let's say, React Router in production. How is this issue not bustling with activity.

And since it's not, is someone sitting on a solution and not sharing it? Lol

@JaffParker
Copy link

NEVERMIND, I slept on this and it's about just providing the /node_modules/ regex in the include array like this (VIte config):

// ...
commonjsOptions: {
  include: ['../common/lib/index.js', /node_modules/],
}
// ...

That first one is my monorepo dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants