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

types(runtime-core): Allow InjectionKey to be used as a valid PropertyKey #5089

Closed
wants to merge 6 commits into from

Conversation

pikax
Copy link
Member

@pikax pikax commented Dec 10, 2021

This allows the InjectionKey to be used as a PropertyKey, useful for the OptionsAPI.

const key = Symbol() as InjectionKey<number>

defineComponent({
 provide: {
   // this now works
   [key]: 'foo' 
 } 
}

This also affects VTU, since the user needs to provide it's own overrides if needed:

const colorKey = Symbol() as InjectionKey<string>

// before 
mount(A, { globals: { provide: { [colorKey as symbol]: ref('#999999') } } 

// after : No need to cast the symbol
mount(A, { globals: { provide: { [colorKey]: ref('#999999') } }

There's a small issue that this change brings, with this change a Symbol cannot be inferred as an InjectionKey

// before worked
const key: InjectionKey<string> = Symbol(); // worked because `InjectionKey` extended `Symbol`

// after 
const key = Symbol() as InjectionKey<string>; // it need to be changed to a more lenient casting

This should lay down some work to support typed injections on the defineComponent when using Options API

Ref: microsoft/TypeScript#46956

Providing validation on provide on defineComponent is harder than expected because when do a keyof it will convert InjectionKey type into a symbol losing the types.

const colorKey = Symbol() as InjectionKey<number>

// No Error because we cannot validate the type correctly :/
mount(A, { globals: { provide: { [colorKey]: ref('#999999') } }

Here's typescript example

declare const InjectionKeySymbol: unique symbol
export type InjectionKey<T> = symbol & { [InjectionKeySymbol]: T }

declare function extractType<T>(opts: T): { [P in keyof T]: T[P] }

const key = Symbol() as InjectionKey<number>

const s = {
    [key]: 11
}

const sa = s[key]
// this should error but it does not
const sr = s[Symbol()]


const t = extractType(s)
const ta = t[key]
// this should error but it does not
const tb = t[Symbol()] // string

playground

@pikax pikax added scope: types 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. labels Dec 10, 2021
@yyx990803
Copy link
Member

yyx990803 commented Dec 12, 2021

This would technically be a (type) breaking change though, it would break existing apps using const key: InjectionKey<string> = Symbol();?

This probably needs to be put in a minor release (where we by convention may ship type breaking changes)

@pikax
Copy link
Member Author

pikax commented Dec 12, 2021

This would technically be a (type) breaking change though, it would break existing apps using const key: InjectionKey<string> = Symbol();?

Yes

@SimonSimCity
Copy link

SimonSimCity commented Apr 19, 2023

Just want to point out that this problem also (as of my knowledge) is at hand when using the function mount() in your tests. An example can be found in auth0/auth0-vue#228 and https://test-utils.vuejs.org/api/#global, where the author of the package defined his key as InjectionKey<Auth0VueClient>. Please let me (or the author of auth0) know if exporting your key as type InjectionKey<T> is a bad decision to begin with.

Edit: For my tests I can use AUTH0_INJECTION_KEY.valueOf() to get the symbol (as AUTH0_INJECTION_KEY is defined as InjectionKey<T>) which at least lets me avoid the casting 😏

# Conflicts:
#	packages/runtime-core/src/apiInject.ts
#	test-dts/inject.test-d.ts
@github-actions
Copy link

github-actions bot commented Oct 7, 2023

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 95.3 kB (+5.21 kB) 36.3 kB (+1.81 kB) 32.7 kB (+1.6 kB)
vue.global.prod.js 153 kB (+6.18 kB) 56.1 kB (+2.08 kB) 49.9 kB (+1.86 kB)

Usages

Name Size Gzip Brotli
createApp 53 kB (+3.33 kB) 20.6 kB (+1.1 kB) 18.8 kB (+978 B)
createSSRApp 56.9 kB (+3.71 kB) 22.2 kB (+1.25 kB) 20.2 kB (+1.11 kB)
defineCustomElement 55.2 kB (+3.33 kB) 21.4 kB (+1.14 kB) 19.4 kB (+986 B)
overall 66.5 kB (+3.38 kB) 25.6 kB (+1.11 kB) 23.3 kB (+1.05 kB)

@yyx990803 yyx990803 changed the base branch from main to minor December 8, 2023 14:26
@yyx990803 yyx990803 added version: minor and removed 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. labels May 31, 2024
@yyx990803
Copy link
Member

/ecosystem-ci run

@vue-bot
Copy link
Contributor

vue-bot commented Aug 1, 2024

📝 Ran ecosystem CI: Open

suite result latest scheduled
language-tools failure failure
nuxt failure success
pinia success success
primevue success success
quasar success success
radix-vue success success
router success success
test-utils failure success
vant failure success
vite-plugin-vue success success
vitepress failure success
vue-i18n failure failure
vue-macros failure success
vuetify failure success
vueuse failure success
vue-simple-compiler success success

yyx990803 added a commit that referenced this pull request Aug 2, 2024
@yyx990803
Copy link
Member

Looks like the constraint of not being able to use const k: InjectionKey<T> = ... is quite disruptive, but I managed to find out a compatible fix in 321d807.

@yyx990803 yyx990803 closed this Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Rejected
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants