Open jderusse opened 7 months ago
Start a new pull request in StackBlitz Codeflow.
This looks like ~a duplicate of~ it is related to https://github.com/vitejs/vite/issues/14048.
Vite community (including me) has recognized this and is working on it! If you have any suggestions, feel free to do so!
I think this issue is about cjs/esm interop. Probably this is a related PR:
Vite now uses object spread in some cases (something like const paper = { ...cjsLib, default: cjsLib }
), but this object spread doesn't include "inherited" properties (in this case cjsLib.__proto__.Color
), so paper.Color
ends up undefined
. Previous simple approach was just const paper = cjsLib
, so it was somewhat magically handling such inheritance.
I updated a reproduction to illustrate this with a bunch of logging https://stackblitz.com/edit/vitejs-vite-qfx5gb?file=main.js
It looks like both dev and build were working previously, so I think rollup handles this case somehow. I guess technically this was a breaking change, but I feel aligning this behavior goes against ESM concept since paper
defined by import * as paper from ...
should be a plain module object without inheritance. Instead, hopefully user code can adjust it by using default import for cjs:
import paper from "paper/dist/paper-core"
Thanks to @hi-ogawa for pointing out. The objects imported by the namespace import may not need to maintain the prototype chain. I think this may be an abnormal behavior of rollup
, which does not apply to vite
. I will temporarily close the fix until confirmation from the rollup
side.
I feel aligning this behavior goes against ESM concept since
paper
defined byimport * as paper from ...
should be a plain module object without inheritance. Instead, hopefully user code can adjust it by using default import for cjs:import paper from "paper/dist/paper-core"
indeed, this fix the issue.
I am also experiencing the same error with one of my project's dependencies. Recently, the import for one of the transitive dependencies have changed in the project and now the default exported module is not working with import * as Stats from 'stats.js'
.
See issue in @react-three/drei
: https://github.com/pmndrs/drei/issues/1865
@react-three/drei
has "allowSyntheticDefaultImports": false
, so it doesn't allow importing the module directly like import Stats from stats.js
.
I have prepared a minimal stackblitz example. The import syntax in the example seems to be correct as TypeScript is able to recognize the constructor but running or building through vite fails.
See example on stackblitz: https://stackblitz.com/edit/vitejs-vite-zbir1r?file=src%2Fmain.ts
The following is an interpretation of the namespace import object form: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/import#module_namespace_object
I think that the error message "is not a constructor" when instantiating a regular object is as expected.
I tried the default create-react-app template that uses webpack and the import seems to work there.
At the same time, I also tested webpack
and found that the error messages it provides are consistent with vite
.
https://stackblitz.com/edit/webpack-webpack-js-org-gnwump?file=src%2Findex.js
Therefore, I think vite
is running as expected, so it's not a problem with vite
. @9inpachi
@XiSenao
The version of webpack used in the example seems quite old.
I was able to make the import work in the latest version of webpack: https://stackblitz.com/edit/webpack-webpack-js-org-akoet6
@XiSenao Thanks for investigating. You closed a PR https://github.com/vitejs/vite/pull/16009 but it might be still better to have a fix since this was technically a breaking change and it seems like a new source of dev/build inconsistency.
Personally I get that namespace import with cjs is very odd in some cases, but if the fix is simple and align with rollup, then I think Vite team will consider it. If Vite team wants to make esm/cjs interop stricter, then that probably needs to postponed to major release.
@bluwy Do you have any thoughts on this issue and XiSenao's PR?
Thanks for discussing this and laying out the issue. I certainly should've looked in the regression earlier 😅 Reading both sides, I think I agree with @XiSenao that this isn't a bug given the behaviour aligns with node and what's documented in MDN. Personally, I'd prioritize build to match dev compared to the other way round, as dev is where one spends most of their time on.
However, I may be persuaded to make a fix instead if other maintainers feel that this is a big breaking change. I'll ping them to hear their thoughts on this. I'd like to start merging the minor PRs today, so if this issue isn't concluded today, the fix may only land in v5.2.
EDIT: Also, would fixing this cause an inconsistency in node SSR in prod? (if the dep is not bundled)
Thanks for discussing this and laying out the issue. I certainly should've looked in the regression earlier 😅 Reading both sides, I think I agree with @XiSenao that this isn't a bug given the behaviour aligns with node and what's documented in MDN. Personally, I'd prioritize build to match dev compared to the other way round, as dev is where one spends most of their time on.
@bluwy I am not sure how types for this are correct in TypeScript. The default export becomes part of the namespace import and there is no error there. And on the other hand, ImportNamespace.default
gives a type error.
According to TypeScript, using export =
syntax to export default modules is preferred and should work everywhere and seems to work with namespace import with the default tsconfig, so I am confused why Vite fails. Is TypeScript in the wrong here?
See reference TypeScript docs: https://www.typescriptlang.org/docs/handbook/declaration-files/templates/module-d-ts.html#:~:text=Note%20that%20using,using%20export%3D%3A
Describe the bug
After upgrading from vite 5.0.12 to 5.1.0, I get a TypeError at runtime (in my browser's console)
paper is the library https://www.npmjs.com/package/paper installed with
Reproduction
https://stackblitz.com/edit/vitejs-vite-szaciy?file=main.js
Steps to reproduce
install paper
use it
See browser's logs
System Info