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(vite): unexptected overwriting for default export fix(#4553) #4596

Merged
merged 3 commits into from Aug 16, 2021
Merged

fix(vite): unexptected overwriting for default export fix(#4553) #4596

merged 3 commits into from Aug 16, 2021

Conversation

hyf0
Copy link
Contributor

@hyf0 hyf0 commented Aug 13, 2021

Description

fix #4553.

I just found the cause, allow me to take a sleep then add some tests and description :).

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@hyf0
Copy link
Contributor Author

hyf0 commented Aug 13, 2021

couldn't sleep :(. For example. A cjs package compile from esm format in node_modules

// node_modules/foo/index.js
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.a = void 0;
var a = function helloA() { };
exports.a = a;
var foo = function hello() { };
exports.default = foo;

the package exports a object like

// cjsFoo
const foo = {
 helloA: function helloA() {},
 default: function hello() {},
__esModule: { value: true },
}

vite will pre-bundle this package to an esm package. simply It will bundle into

// esmFoo
const foo = {
 helloA: function helloA() {},
 default: function hello() {},
}
export { foo as default }

So, when we write something like import foo from 'foo', it will get the right cjsFoo object. To support named export like import { helloA } from 'foo', vite will add some helper statements. For example, import __vite__cjsImport2_foo from "esmFoo"; const helloA = '__vite__cjsImport2_foo'. The details are in #837. Anyway, It's all solve good in static import.

The problem is shown in dynamic import. Let's continue the case.
For import('foo'), vite will transform it into import('foo').then(m => ({ ...m.default, default: m.default })).
The m is like

m = {
  default: { // cjsFoo
      helloA: function helloA() {},
     default: function hello() {},
    __esModule: { value: true },
  }
}

and after then, it will resolve

dynamic-foo = {
   default: { // cjsFoo
     helloA: function helloA() {},
     default: function hello() {},
    __esModule: { value: true },
  },
  helloA: function helloA() {},
   __esModule: { value: true },
}

Thus, It causes the problem in #4553.
The correct output should be

dynamic-foo = {
     helloA: function helloA() {},
     default: function hello() {},
    __esModule: { value: true },
}

@hyf0 hyf0 marked this pull request as ready for review August 13, 2021 17:13
@antfu
Copy link
Member

antfu commented Aug 14, 2021

Could you add some tests for it? Thanks

@antfu antfu added needs test p3-minor-bug An edge case that only affects very specific usage (priority) labels Aug 14, 2021
@@ -15,6 +15,8 @@
"dep-linked": "link:./dep-linked",
"dep-linked-include": "link:./dep-linked-include",
"dep-esbuild-plugin-transform": "link:./dep-esbuild-plugin-transform",
"dep-cjs-compiled-from-esm": "file:./dep-cjs-compiled-from-esm",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It really confusing me that if I use link: instead of file: Vite won't pre-bundle my package. Not sure whether it is a bug too?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe vite gets a real address with no node_modules in the path, so it's not pre-bundle

if (resolved.includes('node_modules') || include?.includes(id)) {
// dependency or forced included, externalize and stop crawling
if (OPTIMIZABLE_ENTRY_RE.test(resolved)) {
depImports[id] = resolved
}

this issue comment analyzes the issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It looks like file: will make npm/yarn copy the package to node_modules rather than link it.

@hyf0
Copy link
Contributor Author

hyf0 commented Aug 14, 2021

@antfu done.

@antfu antfu merged commit c7929ad into vitejs:main Aug 16, 2021
aleclarson pushed a commit to aleclarson/vite that referenced this pull request Nov 8, 2021
vitejs#4596)

* fix(vite): unexptected overwriting for default export fix(vitejs#4553)

* feat: add test

* feat: delete unused package
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

when dependency modules use React.lazy, vite convert more 'default' wrapped the component
4 participants