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

Bug: global property augmentation of $gtm breaks type checks #464

Closed
romansp opened this issue Aug 8, 2024 · 9 comments · Fixed by #467
Closed

Bug: global property augmentation of $gtm breaks type checks #464

romansp opened this issue Aug 8, 2024 · 9 comments · Fixed by #467

Comments

@romansp
Copy link
Contributor

romansp commented Aug 8, 2024

Info

Tool Version
Plugin v3.0.1
Vue v3.4.37
TypeScript v5.5.4
Node v20.14.0
OS mac

vue-gtm uses this way to augment global properties.

vue-gtm/src/index.ts

Lines 202 to 211 in 5268e63

// @ts-expect-error: assume that `vue` already brings this dependency
declare module '@vue/runtime-core' {
// eslint-disable-next-line jsdoc/require-jsdoc
export interface ComponentCustomProperties {
/**
* The Vue GTM Plugin instance.
*/
$gtm: GtmPlugin;
}
}

This differs from the recommended one in Vue 3. Instead vue module should be augmented.

For example vue-router changed their global augmentation in version 4.4.1 from @vue/runtime-core to recommended vue. Caveat is that augmentation should be consistent through the whole codebase otherwise TypeScript fails to resolve types properly.

Recommended action is to update module augmentation in all referenced packages. vuejs/router#2321 (reply in thread)

This seems to be an ongoing issue in Vue ecosystem as now all packages will need to fix their augmentation to use vue instead of @vue/runtime-core. For example unimport did it here unjs/unimport#359

@Shinigami92
Copy link
Contributor

Thanks for the notification about that.
We need to find out in which specific vue version this changed so we can set the peer dependency range.

@Shinigami92
Copy link
Contributor

Okay 🤔 I'm a bit confused, because the interface is still there, and even has in its JSDoc the used way described.

https://github.com/vuejs/core/blob/28db2e69f40e53df84f21ea9e98e9d5d45cd6a60/packages/runtime-core/src/componentPublicInstance.ts#L56-L81

Maybe this is really new and vue needs to update its code?

I would like to make the change when their decision is "stable".

@romansp
Copy link
Contributor Author

romansp commented Aug 9, 2024

Honestly I was myself puzzled when I saw this change landing in vue-router and it really seems that it was always supposed to be vue and not @vue/runtime-core. But I'll do more research on that matter.

For example, vue-i18n fixed their types last month: intlify/vue-i18n#1888.

Also vue-router being probably the most dependent package in Vue apps this will for sure cause a massive disruption across other Vue libraries.

@Shinigami92
Copy link
Contributor

@romansp Just as a temporary workaround, you can use useGtm if you are using Composition API

https://github.com/gtm-support/vue-gtm#using-with-composition-api

@Shinigami92
Copy link
Contributor

@romansp any news? did you e.g. open an issue in vue related to that which you could link here?

@romansp
Copy link
Contributor Author

romansp commented Aug 12, 2024

@Shinigami92 no certain news right now and still trying to confirm. vue-router release notes state the following

Important

This release replaces declare module '@vue/runtime-core' with declare module 'vue' like it's supposed to be. If you are also augmenting @vue/runtime-core, you will likely have to change it to vue. It is also recommended to use an up-to-date TypeScript version (>=5.4) and "moduleResolution": "Bundler" in your tsconfig.json.

I absolutely understand you being extra cautious not to break other people's code but I'm afraid this change will have to be made.

@Shinigami92
Copy link
Contributor

So... I had an internal discussion with some Vue ecosystem persons, and the "issue" is originally coming from vue-tsc and not vue itself
vue-router and Quasar is already using the new strategy as you already described
the documentation (website) changed in vuejs/docs@92f1b4b
but it looks like vue core JSDoc was forgotten

Do you want to open up a PR (so you will also be visible in release notes and commit history) and implement the requested change?

you should update the vue peer dependency to ">= 3.2.26 < 4.0.0"

"vue": ">= 3.2.0 < 4.0.0"

@romansp
Copy link
Contributor Author

romansp commented Aug 13, 2024

Sounds good, I got message back from posva on vue-router Discord confirming the change as well https://discord.com/channels/325477692906536972/325479452773580800/1272810301661777981.

in short, yes use declare 'vue'. The whole situation is a bit more complicated and things error in other cases because of order. There might be another vuex version

Will open PR later today.

romansp added a commit to romansp/vue-gtm that referenced this issue Aug 13, 2024
@romansp
Copy link
Contributor Author

romansp commented Aug 13, 2024

Also started an issue in Vue to fix JSDoc vuejs/core#11605.

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

Successfully merging a pull request may close this issue.

2 participants