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

@vue/compiler-sfc output is not tree-shakeable #2860

Closed
mgdodge opened this issue Dec 21, 2020 · 16 comments
Closed

@vue/compiler-sfc output is not tree-shakeable #2860

mgdodge opened this issue Dec 21, 2020 · 16 comments
Labels
❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. scope: compiler scope: sfc

Comments

@mgdodge
Copy link

mgdodge commented Dec 21, 2020

Version

3.0.4

Reproduction link

https://github.com/mgdodge/rollup-plugin-vue-treeshake-bug-vue3

Steps to reproduce

Originally logged with rollup-plugin-vue, but the tree-shaking issues seem to be coming @vue/compiler-sfc. The following steps are still valid, and still occur with the latest version.

Vue 3 tree-shaking bug

This repo has two branches - one written in Vue 2, one in Vue 3. The components and logic involved are identical (a simple diff of the branches will illustrate this). The only differences are in the dependencies introduced by Vue itself, using defineComponent in Vue 3 instead of Vue.extend for Vue 2, and the inclusion of postCSS for processing the css (required for vue3).

Reproducing the bug

Setup

  • Clone repo
  • Checkout Vue 2 branch
  • Run npm install followed by npm run build and npm pack to create .tgz that can be used in external projects. Rename that file to demolib-vue2.tgz
  • Checkout Vue 3 branch and repeat last step, renaming file to demolib-vue3.tgz
  • Create two sample Vue cli projects, one each for Vue 2 and Vue 3
  • Install cli project dependencies, including the correct .tgz files from the prior steps

Usage

Load 2 of the 3 components provided by the library. Components are Cat, Dog, and Fish.

import { Cat, Dog } from 'demolib'; // No Fish

// For Vue 2
Vue.component('cat', Cat);
Vue.component('dog', Dog);
Vue.mount('#app');

// For Vue 3
const app = createApp(App);
app.component('cat', Cat);
app.component('dog', Dog);
app.mount('#app');
<template>
    <p>Pet options:</p>
    <cat />
    <dog />
    <!-- Fish is not used -->
</template>

Verify tree-shaking

Execute npm run build in each cli project. The resulting "optimized" javascript files will include a chunk-vendors file which should include a tree-shaken version of demolib. Search this file for the following strings:

  • "The cat is named"
  • "The dog is named"
  • "The fish is named"

If tree-shaking worked, you should not see The fish is named anywhere in the output. In the Vue 2 version, tree-shaking works properly. In the Vue 3 version, the unused code is still included.

What is expected?

Code output should be tree-shakeable

What is actually happening?

Code is not tree-shakeable


As noted in this comment on the rollup-plugin-vue bug report, adding /*__PURE__*/ comments to some of the output gets things really close, but there are still some code refactors that are required to get things working.

@HcySunYang
Copy link
Member

I think the root cause is here(According to this comment):

script.render = render;
script.__scopeId = "data-v-8c4b8c4a";

I think we only need to make render and __scopeId as part of the defineComponent function parameters, and no need to add the /*#__PURE__*/ annotation on pushScopeId/popScopeId call, because it has side effects.

@uimijs
Copy link

uimijs commented Jan 11, 2021

你只需要在构建中删除defineComponent
code.replace(/(vue.)*defineComponent\(/gi, "/*#__defineComponent__*/(")

@mgdodge
Copy link
Author

mgdodge commented Jan 30, 2021

Not surprisingly, vite.js libarary mode output, though minified, still suffers from this. Thinking this should get a bump in priority, since more prominent tooling/documentation is now promoting libarary builds which will suffer from this.

@cowills
Copy link

cowills commented Apr 28, 2021

Should this issue be labelled as a bug instead of a feature? Tree shaking seems like a critical feature for vue 3 given the changes to the global API and the switch from options to composition.

@bjkippax
Copy link

bjkippax commented May 6, 2021

Should this issue be labelled as a bug instead of a feature? Tree shaking seems like a critical feature for vue 3 given the changes to the global API and the switch from options to composition.

I'd be inclined to agree here.
Tree-shaking is fantastic and to have it not working is inconvenient.

@yyx990803 yyx990803 added 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. ❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. and removed 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. labels May 28, 2021
@mseele
Copy link

mseele commented Aug 31, 2021

Any progress in this topic? Sadly this is a showstopper to move our private libraries to vue 3, too.

@bjkippax
Copy link

Any progress in this topic? Sadly this is a showstopper to move our private libraries to vue 3, too.

Agreed. It’s causing our organisation problems too.

@ckvv
Copy link

ckvv commented Sep 1, 2021

@mseele @bjkippax
We can support tree-shaking by compiling it into multiple files. like this https://github.com/ckpack/v-ui-template,
And here is a pack I wrote with it https://github.com/ckpack/vue-color

@mgdodge
Copy link
Author

mgdodge commented Sep 13, 2021

@chenkai0520
I've seen this suggested before, and responded here. While it might work, it is a lot more code to maintain and is a regression when moving from vue 2 to vue 3. The intent of this issue is to discover why it's regressed, and hopefully resolve it. In addition, more comments in that thread indicate that the tree-shaking is only working at a "per file" level, not at a "per function" level - so if your file included some functions that go unused, the unused functions are still included in the bundle.

Also, as indicated in this comment above, this bug is also present in vite library output mode. If the "official" tooling for generating libraries suffers from this issue, I would think it need to be fixed properly, or at the very least, the documentation updated to let people know they need to structure their projects differently, as you are suggesting here.

No disrecpect, I'm glad you've got a workaround, and it's valuable to have a way to do things while waiting for this bug to be addressed.

@yyx990803
Copy link
Member

This is now fixed by vitejs/vite@316d7af + cb2d7c0

Will need @vitejs/plugin-vue >= 1.8.1.

vue-loader@next will get similar fix soon.

@bjkippax
Copy link

This is now fixed by vitejs/vite@316d7af + cb2d7c0

Will need @vitejs/plugin-vue >= 1.8.1.

vue-loader@next will get similar fix soon.

Thanks @yyx990803 - greatly appreciated!!

@yyx990803
Copy link
Member

Now also fixed in vue-loader 16.6+ (vuejs/vue-loader@11e3cb8)

@mgdodge
Copy link
Author

mgdodge commented Sep 24, 2021

@yyx990803 , I don't believe this is truly fixed...at least, I can't get it to work. I created another minimal repo to demonstrate - https://github.com/mgdodge/vue-compiler-treeshaking-bug.

The repo uses vite directly instead of any custom rollup configuration. In the demo, I have updated vite to the latest beta, which contains these fixes as far as I can tell.

The repo has a folder for a vite library which should be tree shakeable. It also has two more folders with sample apps (one vite app, one cli app) that attempt to use the library. The README should explain in more detail. Also, the cli app looks like it's using the right version of vue-loader as well.

@yyx990803
Copy link
Member

yyx990803 commented Sep 24, 2021

Ok, so this is because the lib-vue build minifies the dist files, which removes the /*#__PURE__*/ annotations in the generated code. You also forgot to add /*#__PURE__*/ before your own defineComponent calls (I'm not sure why you are doing it in the first place, since you don't need it if you are not using TS, or if you are using <script setup>).

We should probably default to not minifying in lib mode in Vite.

The code generated by @vitejs/plugin-vue is correct and can be treeshaken if you disable minify in lib-vite.

yyx990803 added a commit to vitejs/vite that referenced this issue Sep 24, 2021
@yyx990803
Copy link
Member

Should be fixed by vitejs/vite@06d86e4

@mgdodge
Copy link
Author

mgdodge commented Oct 1, 2021

@yyx990803 thanks for all the work on this - you are amazing! Updated to the latest vite release, and when writing the library in vite, then using in a vite app, the code does in fact seem tree-shakeable. Testing tree-shakeability with npx agadoo also looks good. But when using the library in a vue cli app, tree shaking is not happening.

Digging deeper, right at the bottom of the vite library output, it looks like the following code is the only remaining snag:

var components = /* @__PURE__ */ Object.freeze({
  __proto__: null,

  // Removing the [Symbol.toStringTag] line fixes tree shaking
  [Symbol.toStringTag]: "Module",

  Cat: cat,
  Dog: dog,
  Fish: fish
});

I honestly don't know what that line is for, or if it's strictly necessary. If it is, I wonder what can be done to make things work in webpack - most of the world is still going to be using that for a bit, so it's not exactly something that can be easily ignored...

That code is coming from this line in vite. I think I'll file a bug in the vite repo, since technically it's not a problem with @vue/compiler-sfc any more. I can use a similar setup with rollup-plugin-vue (using @vue/compiler-sfc behind the scenes) and tree-shaking works fine with that output. I just hate to lose all this context.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. scope: compiler scope: sfc
Projects
None yet
Development

No branches or pull requests

9 participants