fix(tpc): custom feature detection flags #113

huang-julien · 2024-06-26T09:44:33Z

🔗 Linked issue

❓ Type of change

📖 Documentation (updates to the documentation or readme)
🐞 Bug fix (a non-breaking change that fixes an issue)
👌 Enhancement (improving an existing functionality)
✨ New feature (a non-breaking change that adds functionality)
🧹 Chore (updates to the build process or auxiliary tools and libraries)
⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

This PR add feature detection for tpc composables. It also removes augmentWindowTypes to make it default

vercel · 2024-06-26T09:44:36Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
scripts-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 11, 2024 7:09pm
scripts-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 11, 2024 7:09pm

huang-julien

@flashdesignory do you need it in other components ?

src/runtime/composables/useFeatureDetection.ts

flashdesignory · 2024-06-26T12:51:53Z

@flashdesignory do you need it in other components ?

once GoogleMaps and Youtube is integrated, we should also add the feature detection there 🙏

flashdesignory

LGTM 🚀

huang-julien · 2024-06-26T13:24:54Z

yes we need to discuss about theses components at the next meeting :)

harlan-zw · 2024-06-26T14:52:59Z

Aren't these already implemented as part of https://github.com/nuxt/scripts/blob/main/src/runtime/composables/useScript.ts#L26 ?

flashdesignory · 2024-06-26T15:23:32Z

we want to differentiate between all nuxt scripts and the third-party-capital integrations.
Additionally, the naming convention that is currently used, diverged from the flags introduced in the initial pr.
This results in a mismatch between nuxt script and chrome flags.

harlan-zw · 2024-06-26T15:55:21Z

we want to differentiate between all nuxt scripts and the third-party-capital integrations.

You can just use the key? ga / gtm is tpc

Additionally, the naming convention that is currently used, diverged from the flags introduced in the initial pr.
This results in a mismatch between nuxt script and chrome flags.

Yes, this was intentional. What / which chrome flags are you referring to?

flashdesignory · 2024-06-26T16:01:01Z

We implemented flags in Chrome that we can use to pull usage data.
These flags currently exist:

nuxt-third-parties-gtm
nuxt-third-parties-ga
nuxt-third-parties-GoogleMaps
nuxt-third-parties-YoutubeEmbed

previous pr: https://github.com/nuxt/third-party-capital/pull/53

harlan-zw · 2024-06-28T04:16:09Z

We implemented flags in Chrome that we can use to pull usage data. These flags currently exist:
nuxt-third-parties-gtm
nuxt-third-parties-ga
nuxt-third-parties-GoogleMaps
nuxt-third-parties-YoutubeEmbed
previous pr: nuxt/third-party-capital#53

Is it possible to update the flags? It would make more sense for the scope prefix to match the module name.

It's a bit clunky to try and support both naming conventions

huang-julien · 2024-07-03T19:40:08Z

any progress on this ?

flashdesignory · 2024-07-03T20:04:23Z

@huang-julien - thanks for following up.

I'd still like to use the updated naming convention for the following reasons:

we are more interested in the third-party-capital integrations
updating flags that are already deployed in chrome is time intense
more granular usage reports available, if we add all Nuxt script flags to chrome in the future as well

Personally, I think the work is doable, since we're not dealing with a large number of third-parties anytime soon.
Keeping both group of flags updated seems reasonable.

huang-julien · 2024-07-03T20:14:03Z

the updated naming convention

Are you talking about nuxt-third-parties-${id} ? @harlan-zw wdyt if we move to this convention in useScript ?

flashdesignory · 2024-07-03T20:24:37Z

the updated naming convention

Are you talking about nuxt-third-parties-${id} ? @harlan-zw wdyt if we move to this convention in useScript ?

correct, the only thing I am not sure about is if the ids match.
We previously used "ga" and "gtm"

huang-julien · 2024-07-03T21:18:50Z

Yes google analytics was using gtag instead of ga. It's now fixed but shouldn't the main script in the data of tpc have the same id used for feature detection ?

@harlan-zw i'm changing the feature detection to nuxt-third-parties-${id}, are you okay with it ?

harlan-zw · 2024-07-04T01:20:46Z

we are more interested in the third-party-capital integrations

This is fine, I don't see how using different keys stops you from doing this.

updating flags that are already deployed in chrome is time intense

It seems like this is just a matter of changing a string but maybe you can clarify further exactly how much work are we talking? Ideally these wouldn't of been set initially until we were close to making the module public, breaking changes should have been expected.

more granular usage reports available, if we add all Nuxt script flags to chrome in the future as well

Not sure how using different keys helps with this.

The reason why I prefer a properly named prefix:

Chrome may not be the only ones using these flags
Clear where the scripts are being loaded from

Keep in mind that even if we merge this, Youtube and Google Maps will still be broken.

flashdesignory · 2024-07-06T21:46:08Z

Maybe I am missing something 🤷.
If I understand the current implementations correctly, a user has two options to use Google Analytics and Google Tag Manager:

the default nuxt script version
the tpc integration (if the user opts into using that).

Is that assumption correct?

If so, and I am using the same flags from nuxt script, how can I differentiate if a user uses 1) or 2) ?
Using the same flag wouldn't give me a correct signal.
Adding an additional flag (with this pr), would allow me to just get the usage of the tpc integrations.

harlan-zw · 2024-07-07T01:34:07Z

We only give one option of which to use, it would be confusing otherwise and add no value to the end user.

GA and GTM is using TPC.

housseindjirdeh · 2024-07-08T18:35:39Z

It seems like this is just a matter of changing a string but maybe you can clarify further exactly how much work are we talking? Ideally these wouldn't of been set initially until we were close to making the module public, breaking changes should have been expected.

You're right, it's a quick string change. The larger concern is that any changes in Chromium will take a while before it hits stable (likely well after this module goes public and gathers adoption). We wanted to get a bit ahead of the curve by naming these strings before the module ships since it can take months to see the change in stable browser.

harlan-zw · 2024-07-11T08:43:12Z

Thanks for clarifying, In this case, I'd consider this PR okay to go ahead with. Do we need to handle GTM as well @huang-julien

huang-julien · 2024-07-11T11:52:32Z

tag manager has already gtm as key in tpc.

harlan-zw · 2024-07-11T11:58:08Z

src/runtime/composables/useScript.ts

@@ -25,7 +25,7 @@ export function useScript<T extends Record<string | symbol, any>>(input: UseScri
    if (!nuxtApp._scripts?.[id]) {
      performance?.mark?.('mark_feature_usage', {
        detail: {
-          feature: `nuxt-scripts:${id}`,
+          feature: `nuxt-third-parties-${id}`,


I'd prefer if the non-TPC scripts used the old key name.

So could we add a config key on NuxtUseScriptOption performanceMarkFeature which can be added as deprecated, then we provide the full key via the TPC scripts

feature: options?.performanceMarkFeature || `nuxt-scripts:${id}`,

i'm good with this 👍

as deprecated ? Maybe as internal if you don't want it to be public

harlan-zw · 2024-07-11T11:58:57Z

src/tpc/utils.ts

@@ -86,7 +87,7 @@ declare global {
  chunks.push(`
 export function ${input.scriptFunctionName}<T extends ${input.tpcTypeImport}>(_options?: Input) {
 ${functionBody.join('\n')}
-  return useRegistryScript${hasParams ? '<T, typeof OptionSchema>' : ''}('${input.tpcKey}', options => ({
+  return useRegistryScript${hasParams ? '<T, typeof OptionSchema>' : ''}('${input.registryId ?? input.tpcKey}', options => ({


we then pass in

performanceMarkFeature: 'nuxt-third-parties-${id}`,

flashdesignory · 2024-07-11T12:10:59Z

@huang-julien - if you're using the key from tpc as the id, do we need to handle the Google Analytics key ?

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/timing/performance_mark.cc;l=157-161?q=nuxt-third-parties&ss=chromium

https://github.com/GoogleChromeLabs/third-party-capital/blob/main/src/third-parties/google-analytics/data.json

I remember you mentioned that these are different and if that's the case, I think we can rename that one in tpc (@housseindjirdeh ).

frat(tpc): add feature detection

68d365f

huang-julien requested review from flashdesignory and harlan-zw June 26, 2024 09:44

huang-julien changed the title ~~frat(tpc): add feature detection~~ feat(tpc): add feature detection Jun 26, 2024

vercel bot deployed to Preview – scripts-playground June 26, 2024 09:44 View deployment

huang-julien commented Jun 26, 2024

View reviewed changes

chore: lint

0686380

vercel bot deployed to Preview – scripts-playground June 26, 2024 09:49 View deployment

vercel bot deployed to Preview – scripts-docs June 26, 2024 09:52 View deployment

flashdesignory reviewed Jun 26, 2024

View reviewed changes

src/runtime/composables/useFeatureDetection.ts Outdated Show resolved Hide resolved

fix: double encoded

4858791

vercel bot deployed to Preview – scripts-playground June 26, 2024 13:13 View deployment

vercel bot deployed to Preview – scripts-docs June 26, 2024 13:16 View deployment

flashdesignory approved these changes Jun 26, 2024

View reviewed changes

huang-julien mentioned this pull request Jun 30, 2024

refactor(tpc): remove augmentWindowTypes #119

Merged

6 tasks

Merge branch 'main' into feat/feature_detection

d4618a1

vercel bot deployed to Preview – scripts-playground July 3, 2024 20:08 View deployment

vercel bot deployed to Preview – scripts-docs July 3, 2024 20:10 View deployment

huang-julien assigned harlan-zw Jul 3, 2024

fix: set ga regsitry key for google analytics

27627ca

vercel bot deployed to Preview – scripts-docs July 3, 2024 21:21 View deployment

vercel bot deployed to Preview – scripts-playground July 3, 2024 21:22 View deployment

chore: remove feature detection composable

b051bca

vercel bot deployed to Preview – scripts-playground July 3, 2024 21:58 View deployment

vercel bot deployed to Preview – scripts-docs July 3, 2024 21:59 View deployment

huang-julien marked this pull request as ready for review July 11, 2024 11:52

harlan-zw reviewed Jul 11, 2024

View reviewed changes

huang-julien marked this pull request as draft July 11, 2024 18:52

feat: add new performanceMarkFeature option

4f7a152

vercel bot deployed to Preview – scripts-playground July 11, 2024 19:02 View deployment

vercel bot deployed to Preview – scripts-docs July 11, 2024 19:05 View deployment

fix: stringify string

1dd4644

vercel bot deployed to Preview – scripts-docs July 11, 2024 19:07 View deployment

vercel bot deployed to Preview – scripts-playground July 11, 2024 19:09 View deployment

huang-julien marked this pull request as ready for review July 11, 2024 20:52

harlan-zw changed the title ~~feat(tpc): add feature detection~~ fix(tpc): custom feature detection flags Jul 12, 2024

harlan-zw approved these changes Jul 12, 2024

View reviewed changes

harlan-zw merged commit 4c35f73 into main Jul 12, 2024
4 checks passed

harlan-zw deleted the feat/feature_detection branch July 12, 2024 03:01

fix(tpc): custom feature detection flags #113

fix(tpc): custom feature detection flags #113

Uh oh!

Conversation

huang-julien commented Jun 26, 2024

🔗 Linked issue

❓ Type of change

📚 Description

Uh oh!

vercel bot commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

huang-julien left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

flashdesignory commented Jun 26, 2024

Uh oh!

flashdesignory left a comment

Choose a reason for hiding this comment

Uh oh!

huang-julien commented Jun 26, 2024

Uh oh!

harlan-zw commented Jun 26, 2024

Uh oh!

flashdesignory commented Jun 26, 2024

Uh oh!

harlan-zw commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flashdesignory commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

harlan-zw commented Jun 28, 2024

Uh oh!

huang-julien commented Jul 3, 2024

Uh oh!

flashdesignory commented Jul 3, 2024

Uh oh!

huang-julien commented Jul 3, 2024

Uh oh!

flashdesignory commented Jul 3, 2024

Uh oh!

huang-julien commented Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

harlan-zw commented Jul 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flashdesignory commented Jul 6, 2024

Uh oh!

harlan-zw commented Jul 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

housseindjirdeh commented Jul 8, 2024

Uh oh!

harlan-zw commented Jul 11, 2024

Uh oh!

huang-julien commented Jul 11, 2024

Uh oh!

harlan-zw Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huang-julien Jul 11, 2024

Choose a reason for hiding this comment

Uh oh!

huang-julien Jul 11, 2024

Choose a reason for hiding this comment

Uh oh!

harlan-zw Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flashdesignory commented Jul 11, 2024

Uh oh!

Uh oh!

Uh oh!

vercel bot commented Jun 26, 2024 •

edited

Loading

harlan-zw commented Jun 26, 2024 •

edited

Loading

flashdesignory commented Jun 26, 2024 •

edited

Loading

huang-julien commented Jul 3, 2024 •

edited

Loading

harlan-zw commented Jul 4, 2024 •

edited

Loading

harlan-zw commented Jul 7, 2024 •

edited

Loading

harlan-zw Jul 11, 2024 •

edited

Loading

harlan-zw Jul 11, 2024 •

edited

Loading