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

docs: revert recent docs changes making gtag a window global #623

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

benmccann
Copy link
Contributor

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests

Description

reverts #570 by @a4vg and #578 by @cornedor

Use cases and why

When I upgraded to the latest partytown I updated my project with the latest changes from the docs as well. When I visited my sight after these changes no hits appeared in Google Analytics until I reverted these changes. The old code had been present and working for a very long time. I don't think the new code works in all circumstances. I have a very basic project with no additional setup besides what is in the docs. Maybe gtag should be set on window like that only in special circumstances?

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • Added new tests to cover the fix / functionality

Copy link

vercel bot commented Sep 4, 2024

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

Name Status Preview Comments Updated (UTC)
partytown ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 4, 2024 7:13pm

@cornedor
Copy link
Contributor

cornedor commented Sep 4, 2024

Do you perhaps create the gtag function from another scripts already running in partytown? Maybe that clashes.

I think it would be better to keep it in the docs, but point out that it is not always necessary.

@cornedor
Copy link
Contributor

cornedor commented Sep 4, 2024

Aside from that, isn't this a bug? window.gtag = function gtag() {} is completely valid.

Copy link
Member

@gioboa gioboa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you let me know if your implementation with the old code currently works correctly? If so, can we update the docs with your code instead of removing it?
Thanks

@benmccann
Copy link
Contributor Author

benmccann commented Nov 18, 2024

Do you perhaps create the gtag function from another scripts already running in partytown?

Nope

Aside from that, isn't this a bug? window.gtag = function gtag() {} is completely valid.

Did you have some typo here? I don't understand what you're saying. window.gtag = function gtag() {} is a bizarre way to write it as it is unnecessary to have the second gtag there. I don't know why the docs have it and it can be removed, but having it there is valid JS and doesn't break anything.

can you let me know if your implementation with the old code currently works correctly?

When I was referring to the old code I mean the previous state of the tutorial. E.g. here you can see what the page looked like 10 months ago: https://github.com/QwikDev/partytown/blob/759fc6b9be77c50094f38ce2c783116578e7bec5/docs/sveltekit.md

If so, can we update the docs with your code instead of removing it?

I'm not sure what you mean. There was new code added that broke it. To go back to the old version means removing the changes that broke it. I don't quite know what else to do

@cornedor
Copy link
Contributor

Did you have some typo here? I don't understand what you're saying. window.gtag = function gtag() {} is a bizarre way to write it as it is unnecessary to have the second gtag there. I don't know why the docs have it and it can be removed, but having it there is valid JS and doesn't break anything.

No, this wasn't a typo. This way, you have to make the least amount of changes for the function to work. I assume this worked for me because without window.gtag = , a Proxy on the window object wouldn't observe the assignment in the global scope. But of course the second gtag isn't strictly necessary.

I'm not sure what you mean. There was new code added that broke it. To go back to the old version means removing the changes that broke it. I don't quite know what else to do

I think it would be great to keep part of it in the docs as reference. The old situation did not work for me, and it took quite a lot of time to figure out what was wrong. I suggest keeping it in the docs with something like: "If a function definition in global scope does not work, you could try to explicitly assign it to window"

@benmccann
Copy link
Contributor Author

What were you referring to when you said "Aside from that, isn't this a bug?"

It would be nice to understand when exactly window.gtag is necessary or breaking rather than advising users to try it both ways and see which one works for them. Can you give an example of what code you were writing that required it?

@cornedor
Copy link
Contributor

What were you referring to when you said "Aside from that, isn't this a bug?"

That small change should not change how that function is working. If it breaks on your project, something seems to be going on.

It would be nice to understand when exactly window.gtag is necessary or breaking rather than advising users to try it both ways and see which one works for them. Can you give an example of what code you were writing that required it?

I'll try to replicate the issue I had, but I have to make some time for this. If I recall correctly, it might have something to do with the gtag function being called from within the worker.

@gioboa
Copy link
Member

gioboa commented Dec 9, 2024

@benmccann is this PR still valid?

@benmccann
Copy link
Contributor Author

Yes, I think so. I'm happy to help get this PR unblocked or to improve it, but it's not clear to me what's required to do so. The current docs are unclear as to why gtag is being recommended to be added to the window variable and doing so had broken Partytown for me. I feel like the docs were simpler before the window recommendations were added and the onus for adding extra steps should include explaining why and when to do so

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants