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

feat: allow react/jsx-dev-runtime imports #7246

Closed
wants to merge 3 commits into from

Conversation

aleclarson
Copy link
Member

Description

This PR contains the following changes:

  • Imports of react/jsx-runtime and react/jsx-dev-runtime are now resolved with the same module ID and globally deduplicated
  • Skip over the actual react/jsx-runtime.js module (which uses process.env.NODE_ENV checking) in favor of manual resolution using the isProduction local variable
  • Ensure both react/cjs/react-jsx-dev-runtime.development.js and react/cjs/react-jsx-runtime.production.min.js are processed by Vite's optimizer

Additional context

Fixes #6215


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@aleclarson aleclarson added p3-minor-bug An edge case that only affects very specific usage (priority) plugin: react labels Mar 9, 2022
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Looks like the lockfile needs an update for the CI to pass. Code lgtm though.

Comment on lines 328 to 329
const devEntry = 'react/cjs/react-jsx-dev-runtime.development.js'
const prodEntry = 'react/cjs/react-jsx-runtime.production.min.js'
Copy link
Member

Choose a reason for hiding this comment

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

With react 18.0.0, it has an exports field that is blocking importing this path, so I'm not sure this works anymore. Perhaps we can optimize react/jsx-runtime and react/jsx-dev-runtime directly? Since the process.env.NODE_ENV should be available in prebundling too.

I also noticed a bug with this Vite code 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Linking to the PR opened after this comment since GitHub didn't link them #7673.

Copy link
Member

Choose a reason for hiding this comment

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

@aleclarson is this PR still valid given @bluwy's message above?

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps we can optimize react/jsx-runtime and react/jsx-dev-runtime directly?

Can't do that because we redirect them to a virtual module:
https://github.com/aleclarson/vite/blob/1069536cb481a8c1c7835d75b0ebeae840fe9bd0/packages/plugin-react/src/index.ts#L374-L378

I think resolving devEntry and prodEntry to absolute paths should work, since that will sidestep the Node.js resolution algorithm (where pkg.exports is enforced).

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we need to provide a virtual module for them? I tested optimizing react/jsx-runtime locally, and I notice that esbuild builds the final output as

var react_jsx_runtime_default = require_jsx_runtime();
export {
  react_jsx_runtime_default as default
};

which breaks named imports, but Vite handles that with the needsInterop flag so it should still work. Unless plugin-react is handling more too?

Copy link
Member Author

Choose a reason for hiding this comment

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

See here:
https://github.com/aleclarson/vite/blob/1069536cb481a8c1c7835d75b0ebeae840fe9bd0/packages/plugin-react/src/index.ts#L380-L397

The motivation is to reduce production bundle size, and we use the same method in development to avoid any production-only surprises.

Copy link
Member

Choose a reason for hiding this comment

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

I see. It feels weird to resolve to a different module in dev/prod since jsx-dev-runtime and jsx-runtime have different exports. Not sure how often that is an issue though.

I guess this is fine then. I also wonder if it's simpler then to use resolveId to resolve react/jsx-dev-runtime => react/jsx-runtime in prod; vice-versa in dev. But either ways they should work the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

It already works that way :)

Of course, the virtual module complicates it a little more.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah my concern is that the virtual module trick adds another layer of complexity, and it's tightly coupled to the named exports. I'll see if the resolveId flow I mentioned above works.

@aleclarson
Copy link
Member Author

Not sure how those failing tests could possibly be related to this PR 🤔

@patak-dev
Copy link
Member

Closing as we have now merged #8546, and test are still failing here. @aleclarson if you think we should continue to discuss the dedupe part of this PR, let's check it in a new PR against main.

@patak-dev patak-dev closed this Jun 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Importing react/jsx-runtime breaks dev builds
3 participants