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(remix-dev/vite): instantiate plugins independently for child vite compiler #7990

Conversation

hi-ogawa
Copy link
Contributor

@hi-ogawa hi-ogawa commented Nov 13, 2023

Closes: #7989

The same issue is already indicated in #7911 (comment) but this time, @vanilla-extract/vite-plugin is affected by the issue since it makes use of config.command = "server" or "build" to switch plugin behavior.
Specifically, the plugin uses this pattern:

export function vanillaExtractPlugin({
  ...
}: Options = {}): Plugin {
  let config: ResolvedConfig;
  ...

  return {
    name: 'vanilla-extract',
    ...
    async configResolved(resolvedConfig) {
      config = resolvedConfig;
      ...
    }

   // ... in later hooks (e.g. transform), it uses `config.command === 'build'` check to switch behavior ...

During vite build, child compiler calls configResolved 2nd time with config.command: "serve", so it forces parent compiler to also behave in the same way.

This PR fixes this issue by loading user vite config file independently for child compiler (via loadConfigFromFile), which will ensure that plugin states won't be mixed between parent and child even though they still have different confg.command.

One issue with this approach is that it also "doubles" remix's own plugin as well, which might cause some issues (or at least negative perf impact). For example, the child compiler is going to have independent resolvePluginConfig cache etc... as it's used for "remix-remove-server-exports" and "remix-react-refresh-babel" plugins which are not removed for child compiler.

Testing Strategy

This PR is tested on reproduction repository mrm007/remix-vite-vanilla-repro#1.
I'll also add a playwright test here.

Copy link

changeset-bot bot commented Nov 13, 2023

⚠️ No Changeset found

Latest commit: a73230e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@hi-ogawa hi-ogawa changed the title fix(remix-dev/vite): instantiate independent plugins for child vite compiler fix(remix-dev/vite): instantiate plugins independently for child vite compiler Nov 13, 2023
@markdalgleish
Copy link
Member

markdalgleish commented Nov 13, 2023

Thanks for the PR! Just a heads up that I've updated your PR since I've migrated the dev branch to the Vite 5 beta, along with a couple of small tweaks.

@markdalgleish
Copy link
Member

markdalgleish commented Nov 14, 2023

This change makes a lot of sense to me. I can see that our current approach to passing plugins to the child compiler is likely to cause issues with random plugins that close over state, and those issues will surface in different ways that make it hard to diagnose or even fix.

The tradeoff here is that the Remix plugin won't be usable if it's passed via inline config to the Vite JS API, but I think that's okay for now. I've added an explicit error message to communicate this limitation. Even if there's a way around this limitation, I'd rather ship your PR now as it leaves us in a better state overall.

I'm happy to merge this PR as-is as long as the current test suite is passing, but feel free to add a Playwright test as a follow-up PR.

@hi-ogawa
Copy link
Contributor Author

Thanks for the review!

The tradeoff here is that the Remix plugin won't be usable if it's passed via inline config to the Vite JS API

Good point, I haven't thought about that. It might be cool if that works, but as you say, I assume making that work is not an immediate concern. I'll think about if this can be also mitigated later.

feel free to add a Playwright test as a follow-up PR.

Sounds good. I'll put this PR ready for review and will create a separate PR to add test cases with vanilla extract plugin.

@hi-ogawa hi-ogawa marked this pull request as ready for review November 14, 2023 01:04
@markdalgleish markdalgleish merged commit 8eb9621 into remix-run:dev Nov 14, 2023
9 checks passed
@hi-ogawa hi-ogawa deleted the fix-vite-avoid-plugin-mutation-by-child-compiler branch November 14, 2023 01:51
Copy link
Contributor

🤖 Hello there,

We just published version 2.3.0-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 2.3.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants