import star works in dev but not in prod #15542
Comments
|
|
|
It seems that |
bluwy
added
p2-edge-case
|
I think that import * as rectClamp from 'rect-clamp' // ✅ dev ❌ prod
const clamped = rectClamp(...)Should have not worked in dev. If you import star |
|
I am confused why import * as rectClamp from 'rect-clamp'
// rewrite
import _rectClamp from 'rect-clamp';
const rectClamp = _rectClamp;vite/packages/vite/src/node/plugins/importAnalysis.ts Lines 1006 to 1009 in 2836276 Has this changed the meaning of the grammar? |
|
The code you linked which points to the function should have a brief description why. Basically for CJS packages, because you don't know the named exports in advanced, esbuild would bundle everything as However, we still want Checking |
|
what I mean to say is why don't we translate it as follows? if (importedName === '*') {
lines.push(`const ${localName} = {
default: ${cjsModuleName}
}`);
}Instead of if (importedName === '*') {
lines.push(`const ${localName} = ${cjsModuleName}`);
}When the properties are directly assigned to function _mergeNamespaces(n, m) {
for (var i = 0; i < m.length; i++) {
const e = m[i];
if (typeof e !== 'string' && !Array.isArray(e)) {
// There is no iterable property in rectClamp
for (const k in e) {
if (k !== 'default' && !(k in n)) {
const d = Object.getOwnPropertyDescriptor(e, k);
if (d) {
Object.defineProperty(n, k, d.get ? d : {
enumerable: true,
get: () => e[k]
});
}
}
} }
}
return Object.freeze(Object.defineProperty(n, Symbol.toStringTag, { value: 'Module' }));
}
function getDefaultExportFromCjs (x) {
return x && x.__esModule && Object.prototype.hasOwnProperty.call(x, 'default') ? x['default'] : x;
}
_mergeNamespaces({
__proto__: null,
default: getDefaultExportFromCjs(rectClamp)
}, [rectClamp])If the default behavior is not modified, I think |
|
It's because the result of I'm guessing the fix is to check if it has |
|
How about keeping in sync with rollup? _mergeNamespaces({
__proto__: null,
default: getDefaultExportFromCjs(rectClamp)
}, [rectClamp])or the behavior of vite/packages/vite/src/node/plugins/importAnalysis.ts Lines 870 to 876 in 9fc5d9c Change the interactive logic of if (importedName === '*') {
lines.push(`const ${localName} = ${cjsModuleName} && ${cjsModuleName}.__esModule ? ${cjsModuleName} : ({ ...${cjsModuleName}, default: ${cjsModuleName} })`)
} |
|
Keeping in sync with what rollup does seems like the safest approach. |
|
I am thinking about a question: why doesn't |
This is what we did in Vite 3. We had experimental support for pre-bundling with esbuild during build these past years, but we decided to remove the feature. See the rationale here: The tl;dr; is that we are going to try to go in the other way around to reduce inconsistencies, with Rolldown (that is compatible with rollup) replacing esbuild during dev for pre-bundling. |
Sorry for not following this up. Been swamped 😢 Personally I'd go with your second option and not syncing with Rollup, unless the Plus, the interop code is mainly to be compatible with esbuild's output. The rollup code is meant to be used on the raw files directly I think? Esbuild's output already partially handles CJS, so we can add a bit more code to make the compatibility work. Ultimately, would the final interop code be something like this? const ${localName} = typeof cjsModuleName === 'object' : cjsModuleName : { default: ${cjsModuleName} } |
|
Thank you for your reply!
I agree, the logic implemented by the helper code is not complicated, and directly injecting it into the original module does not seem to create a significant burden.
This problem is a bit tricky, I personally think that the output forms of
In other words, we may also need to support the following equation. import * as a from "demo";
import("demo").then(b => {
// true
console.log(a === b);
})I am not sure if what you are expressing means that satisfying the output of the development phase is sufficient as a subset of the output of the build phase. |
I think if we can ensure that the shape of namespace imports and dynamic imports are the same, then it should be a good start. We don't have to make sure they're strictly equal. I guess reading about this more:
This one sounds like the best fix for me if it also fixes the issue.
I mean the interop code we're dealing with is mainly dev exclusive only. Build has its own different path. In dev, we're using esbuild to prebundle, so we should adapt to its output and make it consistent to how it'll work in build. Build doesn't use the prebundled output anymore, especially with #15184. We should still implement a fix that makes dev and build consistent, but we don't have to copy over what build does completely to achieve the consistency. |
Yeah, originally I was considering directly using the output behavior of // module main
module.exports = [1, 3, 4]
// index.js
import("main").then((dynamicImport) => {
/**
* !!! Bad Case !!!
* dev output: { 0: 1, 1: 3, 2: 4, default: [1, 3, 4] }
* build output: { default: [1, 3, 4] }
*/
console.log(dynamicImport)
})
Yes, referencing equality in strict mode is still an issue. In fact, I wanted to discuss with you yesterday whether support for this is needed. If necessary, I feel that I can only create a cache layer to ensure that the references loaded and obtained in other modules remain consistent. I seem to have not thought of a better way to handle the issue of inconsistent citations. |
|
Interesting. I guess we can add additional code to prevent that from happening. Even though it'll be very similar to Rollup's implementation, I think my main concern is that #15600 is completely porting Rollup's implementation into Vite. We can slowly adjust our interop code where needed, but I don't think we need to go that far to be exactly compatible, e.g. the |
|
I understand what you mean, so let's make it compatible through the following logic. @bluwy const translate = (module) => {
if (Array.isArray(module) || typeof module === 'string') {
return {
default: module
}
}
if (module && module.__esModule) {
return module;
}
return ({ ...module, default: module });
} |
|
If we do that, could we avoid the helper and new virtual module? Even if it is a bit more verbose, it should still be fine. |
Describe the bug
I am using an npm module that does not have TypeScript definitions. I imported it like so:
And it works fine in development. But when I build my code, I get a runtime error that the function is not defined. Changing the import like so works fine:
Reproduction
https://stackblitz.com/edit/vitejs-vite-kgbyd3?file=src%2Fmain.ts
Steps to reproduce
Error: Nonenpm run builddistlocallyError: TypeError: y is not a functionSystem Info
On StackBlitz: System: OS: Linux 5.0 undefined CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz Memory: 0 Bytes / 0 Bytes Shell: 1.0 - /bin/jsh Binaries: Node: 18.18.0 - /usr/local/bin/node Yarn: 1.22.19 - /usr/local/bin/yarn npm: 9.4.2 - /usr/local/bin/npm pnpm: 8.14.0 - /usr/local/bin/pnpm npmPackages: vite: ^5.0.8 => 5.0.11Validations
The text was updated successfully, but these errors were encountered: