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

[Feature]: Improve treeshaking support with esbuild #808

Open
1 of 2 tasks
jsmnbom opened this issue Apr 1, 2024 · 0 comments
Open
1 of 2 tasks

[Feature]: Improve treeshaking support with esbuild #808

jsmnbom opened this issue Apr 1, 2024 · 0 comments

Comments

@jsmnbom
Copy link
Contributor

jsmnbom commented Apr 1, 2024

Describe the feature

Currently when using radix-vue in a project that is built using esbuild tree shaking does not work very well due to various reasons. This can be easily seen by bundling a simple file like:

// @filename simple.ts
import { Primitive } from 'radix-vue'
console.log(Primitive)

On my machine this will currently create a 75kB file, as seen in the below analysis:

npx esbuild --bundle --outfile=dist/simple.js --format=esm --minify --analyze simple.js
  dist/simple.js                                                                                                  75.5kb  100.0%
   ├ node_modules/.pnpm/[email protected][email protected]/node_modules/radix-vue/dist/index.js                           32.2kb   42.6%
   ├ node_modules/.pnpm/@[email protected]/node_modules/@vue/runtime-core/dist/runtime-core.esm-bundler.js  11.0kb   14.5%
   ├ node_modules/.pnpm/@[email protected]/node_modules/@vue/reactivity/dist/reactivity.esm-bundler.js        10.6kb   14.0%
   ├ node_modules/.pnpm/@[email protected]/node_modules/@floating-ui/core/dist/floating-ui.core.mjs           7.5kb   10.0%
   ├ node_modules/.pnpm/@[email protected]/node_modules/@floating-ui/dom/dist/floating-ui.dom.mjs              6.3kb    8.4%
   ├ node_modules/.pnpm/@[email protected]/node_modules/@floating-ui/utils/dist/floating-ui.utils.dom.mjs    2.0kb    2.7%
   ├ node_modules/.pnpm/@[email protected]/node_modules/@vue/shared/dist/shared.esm-bundler.js                     2.0kb    2.6%
   ├ node_modules/.pnpm/@[email protected][email protected]/node_modules/@floating-ui/vue/dist/floating-ui.vue.mjs   1.7kb    2.3%
   ├ node_modules/.pnpm/@[email protected]/node_modules/@floating-ui/utils/dist/floating-ui.utils.mjs        1.4kb    1.9%
   └ simple.js                                                                                                      16b     0.0%

The reason for this huge size turns out to be due to evanw/esbuild#3392 as well as two single statements in https://github.com/radix-vue/radix-vue/blob/main/packages/radix-vue/src/Menu/MenuContentImpl.vue and https://github.com/radix-vue/radix-vue/blob/main/packages/radix-vue/src/Popper/PopperContent.vue.

The best workaround I have found is to move the destructure into the <script> inside a IIFE as suggested in the issue above.

Example diff of putting default props inside a IIFE.
diff --git a/packages/radix-vue/src/Popper/PopperContent.vue b/packages/radix-vue/src/Popper/PopperContent.vue
index d32eaae1..288a3027 100644
--- a/packages/radix-vue/src/Popper/PopperContent.vue
+++ b/packages/radix-vue/src/Popper/PopperContent.vue
@@ -137,6 +137,10 @@ export interface PopperContentContext {
 
 export const [injectPopperContentContext, providePopperContentContext]
   = createContext<PopperContentContext>('PopperContent')
+
+const defaultProps = /* @__PURE__ */ (() => ({
+  ...PopperContentPropsDefaultValue
+}))()
 </script>
 
 <script setup lang="ts">
@@ -167,9 +171,7 @@ defineOptions({
   inheritAttrs: false,
 })
 
-const props = withDefaults(defineProps<PopperContentProps>(), {
-  ...PopperContentPropsDefaultValue,
-})
+const props = withDefaults(defineProps<PopperContentProps>(), defaultProps)
 const emits = defineEmits<{
   placed: [void]
 }>()

This will in turn lead to a 32kB file as the simple output from above.

npx esbuild --bundle --outfile=dist/simple.js --format=esm --minify --analyze simple.js
  dist/simple.js                                                                                                                     32.6kb  100.0%
   ├ ../forks/radix-vue/node_modules/.pnpm/@[email protected]/node_modules/@vue/reactivity/dist/reactivity.esm-bundler.js        10.2kb   31.2%
   ├ ../forks/radix-vue/packages/radix-vue/dist/index.js                                                                             10.2kb   31.2%
   ├ ../forks/radix-vue/node_modules/.pnpm/@[email protected]/node_modules/@vue/runtime-core/dist/runtime-core.esm-bundler.js   9.5kb   29.1%
   ├ ../forks/radix-vue/node_modules/.pnpm/@[email protected]/node_modules/@vue/shared/dist/shared.esm-bundler.js                     2.0kb    6.0%
   └ simple.js                                                                                                                         16b     0.0%

I can submit a PR with this change, but I chose to open an issue since I don't know if this is the way you would like to fix this issue. Alternatively the popper props can just be put right into the defineProps statement: const props = withDefaults(defineProps<PopperContentProps>(), PopperContentPropsDefaultValue) but that excludes the possibility of adding more props in the future to MenuContentImpl that do not also apply to PopperContent - tho maybe that's okay?

Another treeshaking issue comes from the use of createContext. Since createContext is used inside plain <script> tags in .vue files; export const [injectPopperContentContext, providePopperContentContext] = createContext<PopperContentContext>('PopperContent'). The statement will be added verbatim in the bundle (although minified), and since it is a destructure statement esbuild will not be able to treeshake it. I propose instead to export an object from createContext with a inject and a provide property. This will move all property access on the return of the createContext function into actual component setup functions, and will therefore be able to be properly tree shaken. This is a bigger task and would also be backwards incompatible since createContext is exported. I did some tests in https://github.com/jsmnbom/radix-vue/tree/improve-treeshaking and the results do speak for themselves:

npx esbuild --bundle --outfile=dist/simple.js --format=esm --minify --analyze simple.js
 dist/simple.js                                                                                                                     19.0kb  100.0%
   ├ ../forks/radix-vue/node_modules/.pnpm/@[email protected]/node_modules/@vue/reactivity/dist/reactivity.esm-bundler.js         9.4kb   49.6%
   ├ ../forks/radix-vue/node_modules/.pnpm/@[email protected]/node_modules/@vue/runtime-core/dist/runtime-core.esm-bundler.js   4.7kb   24.5%
   ├ ../forks/radix-vue/packages/radix-vue/dist/index.js                                                                              2.3kb   12.4%
   ├ ../forks/radix-vue/node_modules/.pnpm/@[email protected]/node_modules/@vue/shared/dist/shared.esm-bundler.js                     1.7kb    9.2%
   └ simple.js                                                                                                                         16b     0.1%

Note that rollup currently does not seem to think that spread statements or destructures can have side effects (rollup/rollup#3984), but should that ever get fixed then this issue will show up with rollup builds too.

See https://github.com/jsmnbom/radix-vue/tree/improve-treeshaking for the changes I did, and do say if a PR is welcome with the changes, or if this should be handled differently :)

Additional information

  • I intend to submit a PR for this feature.
  • I have already implemented and/or tested this feature.
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

1 participant