gtm-support / vue-gtm

Simple implementation of Google Tag Manager for Vue
https://www.npmjs.com/package/@gtm-support/vue-gtm
MIT License
212 stars 27 forks source link

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

Closed romansp closed 3 months ago

romansp commented 3 months ago

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.

https://github.com/gtm-support/vue-gtm/blob/5268e639f2442f6f0bb2e05f8c7175e73a7826d7/src/index.ts#L202-L211

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. https://github.com/vuejs/router/discussions/2321#discussioncomment-10240858

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 https://github.com/unjs/unimport/pull/359

Shinigami92 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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: https://github.com/intlify/vue-i18n/pull/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 commented 3 months ago

@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 commented 3 months ago

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

romansp commented 3 months ago

@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 commented 3 months ago

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 https://github.com/vuejs/docs/commit/92f1b4bfef37fbdfa473c3fcb745420a107a7de5 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" https://github.com/gtm-support/vue-gtm/blob/5268e639f2442f6f0bb2e05f8c7175e73a7826d7/package.json#L92

romansp commented 3 months ago

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 commented 3 months ago

Also started an issue in Vue to fix JSDoc https://github.com/vuejs/core/issues/11605.