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): set same config.mode for child compiler #7911

Merged
merged 7 commits into from
Nov 9, 2023

Conversation

hi-ogawa
Copy link
Contributor

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

Closes: #7901

I wrote a longer explanation here while investigating it #7901 (comment)
This specific issue of mdx rollup plugin is due to that single plugin instance's hook { config(config, env) => .. } is run twice with different env.mode and child compiler's config hook runs later and it overwrites plugin's internal state which then affects parent compiler's transform https://github.com/mdx-js/mdx/blob/efff74e48aec9286c62185bf04f7b8aa72e07bd6/packages/rollup/lib/index.js#L76-L82

Note that this change only tries to align env.mode between parent and child compilers.
env.command is still inconsistent during vite build where parent has command = "build" while child has command = "serve".
So, this change probably doesn't help more complicated case and thus this is nothing but quick workaround for mdx plugin.
Nonetheless, starting with having consistent env.mode won't probably do any harm, so that's my main rational for this PR.

  • Docs
  • Tests

Testing Strategy:

Confirmed this fix the reproduction from #7901 by patch-package @remix-run/dev with a following patch:

diff --git a/node_modules/@remix-run/dev/dist/vite/plugin.js b/node_modules/@remix-run/dev/dist/vite/plugin.js
index 3f025ab..bcbad86 100644
--- a/node_modules/@remix-run/dev/dist/vite/plugin.js
+++ b/node_modules/@remix-run/dev/dist/vite/plugin.js
@@ -340,6 +340,7 @@ const remixVitePlugin = (options = {}) => {
       await esModuleLexer.init;
       viteChildCompiler = await vite.createServer({
         ...viteUserConfig,
+        mode: viteConfig.mode,
         server: {
           ...viteUserConfig.server,
           // when parent compiler runs in middleware mode to support

I wasn't sure if this PR warrants new test to verify the behavior.
Please let me know if it's desired to add test (for example, integration test with @mdx/rollup for both dev/build, or using dummy plugin like in #7901 (comment) to verify this specific env.mode behavior).

Copy link

changeset-bot bot commented Nov 5, 2023

🦋 Changeset detected

Latest commit: 0a30c7b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@remix-run/dev Patch
create-remix Patch
remix Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/css-bundle Patch
@remix-run/deno Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/testing Patch

Not sure what this means? Click here to learn what changesets are.

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

@hi-ogawa hi-ogawa marked this pull request as ready for review November 5, 2023 02:11
@MichaelDeBoey MichaelDeBoey changed the title fix(vite): set same config.mode for child compiler fix(remix-dev/vite): set same config.mode for child compiler Nov 6, 2023
Copy link
Member

@markdalgleish markdalgleish left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I've added some tests too. I went with MDX-specific tests for this so that we're also testing MDX can be used as routes.

@hi-ogawa
Copy link
Contributor Author

hi-ogawa commented Nov 9, 2023

Thanks for adding tests. Looks good!.

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