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

Vite 3.1.5 breaks Jotai babel plugins #1475

Closed
gunters63 opened this issue Oct 11, 2022 · 16 comments
Closed

Vite 3.1.5 breaks Jotai babel plugins #1475

gunters63 opened this issue Oct 11, 2022 · 16 comments

Comments

@gunters63
Copy link

Using the debug label and react refresh plugins does not work anymore starting with Vite 3.1.5.

vite dev chokes with the following error:

image

Reverting to Vite 3.1.4 fixes the error.

For reproduction use this repo: https://github.com/gunters63/jotei-vite-import-error

pnpm install and pnpm dev should be enough.

I am not sure this is an upstream error in Vite, they had a similar problem which was supposed to be fixed in 3.16.
This problem here persists with 3.1.6 and 3.17.

@gunters63
Copy link
Author

The repo is created with the Vite typescript react template. Then just vite.config.ts was modified to look like here: https://jotai.org/docs/guides/vite

@gunters63
Copy link
Author

Hmm looking at the exports in jotais package.json I am wondering why Vite loads the .js and not the .mjs file (in which the import syntax is legal).

    "./babel/plugin-react-refresh": {
      "types": "./babel/plugin-react-refresh.d.ts",
      "module": "./esm/babel/plugin-react-refresh.js",
      "import": "./esm/babel/plugin-react-refresh.mjs",
      "default": "./babel/plugin-react-refresh.js"
    },

@gunters63
Copy link
Author

Patching package.json and changing the "module" entries of debug-label and react-refresh to point to the .mjs files makes the error go away. This is not a solution of course.

Something in Vite 3.1.5 changed the exports loading logic it seems.

@gunters63
Copy link
Author

Looking at the changes in Vite from 3.1.4 to 3.1.5 this commit changed the module resolving behaviour:

vitejs/vite@dc140af

@dai-shi
Copy link
Member

dai-shi commented Oct 11, 2022

(Hmmm, I follow this pattern in almost all of my libs.)

@gunters63
Copy link
Author

I am just debugging a vite dev call:

image

@gunters63
Copy link
Author

tryNodeResolve returns the .js path, not the .mjs path.

@gunters63
Copy link
Author

I traced execution now into a third party-module called es-module-lexer which Vite seems to use since 3.1.5 for analysing imports/exports etc..

https://github.com/guybedford/es-module-lexer

@gunters63
Copy link
Author

I traced into es-module-lexer now and he bailed out at conditional export "module" which is wrong.
Jotais package.json doesn't have "type": "module" set so all .js files are treated as CSM, hence the illegal import.

I am not 100% sure but I think "module" is not a built-in Node conditional export condition. I also didn't find it in the Typescript documentation.

See: https://nodejs.org/api/packages.html#conditional-exports

Should it probably be "require"? But the file "module" points to contains import statements.
Alternatively adding "type": "module" could also work (but that may have other implications of course).

@gunters63
Copy link
Author

Ah I see, "module" is a custom conditional export for webpack/rollup/wmr.
But here it seems to break Vites export resolution.

@dai-shi
Copy link
Member

dai-shi commented Oct 11, 2022

It's an unofficial/conventional field by community. Same for "types".
It helps when app code is CJS. It's like "module" field under the root in package.json, which is also conventional. It's sometimes known as Babel ESM (not equal to native ESM). So, I think vite should pick "import" instead of "module".

Related PRs: #679 #583 c6d54f7
#289 (comment) is a bit old discussion.

@gunters63
Copy link
Author

Ok, I will make a bug report in the Vite repo and backlink to this discussion. Lets see what happens :)

@benmccann
Copy link

Ah I see, "module" is a custom conditional export for webpack/rollup/wmr.
But here it seems to break Vites export resolution.

Why would you expect webpack/rollup/wmr to use the field, but want Vite to ignore it?

It's an unofficial/conventional field by community

From which community? I'm curious where this originated

It helps when app code is CJS. It's like "module" field under the root in package.json, which is also conventional. It's sometimes known as Babel ESM (not equal to native ESM).

Helps how? What's the point in having two exactly equivalent files with different file extensions?

@dai-shi
Copy link
Member

dai-shi commented Oct 12, 2022

I assume it's originated by webpack and some other bundlers follow it.
We actually once used .mjs in "module" field and someone reported an issue. Old bundlers don't understand .mjs extension. #679 and #677

Currently, they are the same file, but in the future they can be different. Like, if old bundlers don't understand something of native ESM. (But, before reaching to that point, we may drop support of old bundlers, which means to remove "module" field entirely.)

aleclarson added a commit to alloc/resolve.exports that referenced this issue Nov 13, 2022
It is a deprecated convention, meant for tools that do not understand the .mjs file extension.

More context:
  pmndrs/jotai#1475 (comment)
@ahungrynoob
Copy link
Contributor

ahungrynoob commented Dec 3, 2022

Maybe can use https://github.com/ahungrynoob/jotai-ts-transformer. It is a typescript transformer.

@gunters63
Copy link
Author

This problem was fixed already upstream in Vite. @dai-shi: Bug can be closed :)

@dai-shi dai-shi closed this as completed Dec 4, 2022
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

No branches or pull requests

4 participants