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

Fix ESM bundle relying on named imports from react-is #3367

Closed

Conversation

developit
Copy link

This PR fix styled-components' ESM bundles so that they don't rely on named imports react-is.
From a correctness standpoint, it's generally best to avoid relying on named export inference.
More importantly though, react-is doesn't support inference because it conditionally defines module.exports based on build-time constants.

The solution I've implemented in this PR is essentially applying the recommendation from #3256 (passing namedExports to the commonjs plugin). The difference is that it is applied to the styled-components bundles themselves, rather than requiring everyone include the workaround in their own config.

With this change, the ESM bundles now import react-is in a way that works in environments with even the most basic CommonJS-ESM interop:

import e from 'react-is';
const { typeOf, isElement, etc } = e;

Fixes #3366.

@kitten
Copy link
Member

kitten commented Jan 8, 2021

@probablyup Is it worth it to finally drop react-is as we've discussed a while ago? I don't see any code that is explicitly relying on it anymore per se.

There's one section for development warning purposes that isn't effective anymore, i.e. it looks flawed since it checks for an element (typeof x === 'object') while the check is for functions:

if (process.env.NODE_ENV !== 'production' && isElement(result)) {
// eslint-disable-next-line no-console
console.error(
`${getComponentName(
chunk
)} is not a styled component and cannot be referred to via component selector. See https://www.styled-components.com/docs/advanced#referring-to-other-components for more details.`
);
}

The other usage checks for the input type to constructWithOptions and essentially just does typeof type === 'string' || typeof type === 'function':

if (!isValidElementType(tag)) {
return throwStyledError(1, String(tag));
}

It may be effective in theory since we're trying to include React.memo and React.forwardRef but React itself would eventually throw an error about these kinds of mistakes anyway, and I don't see the cost of react-is and the problems it causes as worth it in relation anymore 😅

@@ -28,7 +28,44 @@ const esm = {
const getCJS = override => ({ ...cjs, ...override });
const getESM = override => ({ ...esm, ...override });

/**
* Support external dependencies on CommonJS modules without relying on named export inference
* (works similarly to more recent versions of Rollup's commonjs plugin)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • (works similarly to more recent versions of Rollup's commonjs plugin)

Should we just update the CJS plugin?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could, but that would require updating Rollup 2 major versions forward, which is a pretty major change (everything broke when I tried).

@quantizor
Copy link
Contributor

Dropped this dep in 8165cbe :)

@quantizor quantizor closed this May 11, 2021
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

Successfully merging this pull request may close these issues.

Incorrect usage of react-is from ESM bundles
3 participants