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): fix react-refresh/babel plugin resolution #7904

Conversation

hi-ogawa
Copy link
Contributor

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

Closes: #7905

I think this error is caused by the combination of:

  • babel does its own resolution when plugins option is passed as a string (not sure what rule exactly...)
  • when running custom server (by node ./server.mjs), the base of such resolution becomes user's root directory.
  • react-refresh package (transitive dependency of @remix-run/dev) is not visible from root directory when using pnpm's non-flat node_modules,

Looking at vite-plugin-react, they indeed resolve plugin library on its own then pass to plugins option of babel.transformAsync (see "additional notes" below).
So this PR align with it in a similar way by directly calling require("react-refresh/babel") within @remix-run/dev.

I think the reason why this issue doesn't manifest when running a server via remix cli is that, in that case, the babel plugin resolution starts from @remix-run/dev package directory and node_modules/react-refresh dependency is visible from that directory.

(Additional notes)

vite-plugin-react currently does dynamic import with own caching to resolve babel plugin https://github.com/vitejs/vite-plugin-react/blob/b255776f75ced38e9ae8d0ba2f9113e697159a73/packages/plugin-react/src/index.ts#L197, but they were also using require("react-refresh/babel") before package migration done in vitejs/vite-plugin-react@e999b23 (see the deleted file packages/plugin-react/src/index.js).

Testing Strategy:

I verified this fix the issue on pnpm by applying a patch in the reproduction example via pnpm patch / patch-commit:

diff --git a/dist/vite/plugin.js b/dist/vite/plugin.js
index 3f025ab178692932a2c3b6527d5d82a60021f8b6..129c83032a14cf35a815141d7e1666cc6a14ef0d 100644
--- a/dist/vite/plugin.js
+++ b/dist/vite/plugin.js
@@ -551,7 +551,7 @@ const remixVitePlugin = (options = {}) => {
           allowAwaitOutsideFunction: true,
           plugins: ["jsx", "typescript"]
         },
-        plugins: ["react-refresh/babel"],
+        plugins: [require("react-refresh/babel")],
         sourceMaps: true
       });
       if (result === null) return;

Copy link

changeset-bot bot commented Nov 4, 2023

🦋 Changeset detected

Latest commit: 7f803e0

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 4, 2023 08:10
@hi-ogawa hi-ogawa changed the title fix(vite): fix react-refresh/babel plugin resolution error when custom server with pnpm fix(vite): fix react-refresh/babel plugin resolution error when custom server with pnpm during development Nov 5, 2023
@hi-ogawa
Copy link
Contributor Author

hi-ogawa commented Nov 5, 2023

I see firefox integration test failed on CI, but I cannot reproduce it locally:

$ yarn test:integration -- integration/vite-dev-express-test.ts --project firefox
...
Running 1 test using 1 worker
  1 passed (9.4s)

EDIT: It seems CI passed after re-run.

@MichaelDeBoey MichaelDeBoey changed the title fix(vite): fix react-refresh/babel plugin resolution error when custom server with pnpm during development fix(remix-dev/vite): fix react-refresh/babel plugin resolution Nov 6, 2023
@MichaelDeBoey MichaelDeBoey force-pushed the fix-vite-custom-server-react-refresh-babel-resolution branch from b1248ab to 0e96075 Compare November 6, 2023 01:05
Copy link
Contributor

@pcattori pcattori left a comment

Choose a reason for hiding this comment

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

@hi-ogawa thanks, looks great!

@pcattori pcattori merged commit 8543efe into remix-run:dev Nov 7, 2023
9 checks passed
@hi-ogawa hi-ogawa deleted the fix-vite-custom-server-react-refresh-babel-resolution branch November 8, 2023 01:00
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