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

refactor: remove const enum #9243

Closed
wants to merge 1 commit into from
Closed

Conversation

yangmingshan
Copy link
Contributor

const enum caused many trouble for both user (#1228) and Vue itself. So this PR is trying to remove all the const enum in Vue codebase, with non-breaking way of course.

For types I will use union type to replace const enum, for runtime I will replace const enum with literal value. It's not a breaking change because const enum is compatible with union type, so the code below can continue to work:

const enum TrackOpTypes {
  GET = 'get',
  HAS = 'has',
  ITERATE = 'iterate'
}

type TrackOpTypesUnion = 'get' | 'has' | 'iterate'

function track(target: object, type: TrackOpTypesUnion, key: unknown) {
  // ...
}

track(rawTarget, TrackOpTypes.GET, key) // NO ERROR!

Currently I only refactored the @vue/reactivity package as an example, and it works. I kept the const enum declaration and its exports for compatibility.

Do you think this is worth a try? If so, please let me know so I can continue. Or if you don't think this is the right path, I'll stop there.

@github-actions
Copy link

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 85.9 kB 32.6 kB 29.5 kB
vue.global.prod.js 132 kB 49.3 kB 44.4 kB

Usages

Name Size Gzip Brotli
createApp 47.9 kB 18.8 kB 17.2 kB
createSSRApp 50.6 kB 19.9 kB 18.2 kB
defineCustomElement 50.3 kB 19.6 kB 17.9 kB
overall 61.2 kB 23.7 kB 21.6 kB

@sxzz sxzz added need discussion need guidance The approach/solution in the PR is unclear and requires guidance from maintainer to proceed further. and removed need discussion labels Sep 18, 2023
@sxzz
Copy link
Member

sxzz commented Sep 21, 2023

Thanks for your contribution. After the team discussion, we think we can keep enums but change them from const enum to enum. For internal usage, we can have a rollup plugin to transform the enum reference to raw value. For example, we replace TriggerOpTypes.SET with 'set'. This is for reducing bundle size and taking into account DX.

@yangmingshan
Copy link
Contributor Author

That is very clever, enum to users, const enum to Vue. Impressive!

Should I close this PR?

@sxzz
Copy link
Member

sxzz commented Sep 21, 2023

You can do it in this PR or redo in a new one. It's up to you ❤️

@yangmingshan
Copy link
Contributor Author

I'll open another PR, thank you.

@yangmingshan
Copy link
Contributor Author

@sxzz If we use this approach, we will need contain enum in bundle, otherwise user cannot use these enum at runtime, but it will increase the bundle size. Is it ok?

@sxzz
Copy link
Member

sxzz commented Sep 21, 2023

It’s ok. If it is not referenced, it will be removed by tree-shaking.

@yangmingshan
Copy link
Contributor Author

Not all of them, some enum will be exported, tree-shaking won't help, bundle size will increase a little.

But you can see the results first and then decide.

@xiaoxiangmoe
Copy link
Contributor

@yangmingshan hi, I create a plugin to inline enum, see https://github.com/vuejs/core/pull/9261/files#diff-52a2558e5292b080df0a8a6b519b0ed62af0605833abe1766af51524e1985a34.

Maybe you can refer to this to continue improving your PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need guidance The approach/solution in the PR is unclear and requires guidance from maintainer to proceed further. wait changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants