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

fix: solve error with anchor without href #523

Merged
merged 1 commit into from
Dec 29, 2023

Conversation

franpeza
Copy link
Contributor

@franpeza franpeza commented Dec 29, 2023

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests

Description

An error is thrown when trying to access a property of an anchor element that hasn't been set an href. This PR fixes this issue handling the scenario and returning an appropriate value depending on the property accessed.

This is how the error looks like:

Screenshot 2023-12-29 at 12 54 10

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 Dec 29, 2023

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 Dec 29, 2023 0:13am

@franpeza franpeza marked this pull request as ready for review December 29, 2023 11:59
@SirJalias
Copy link

SirJalias commented Dec 29, 2023

Hello
I consider this PR might help fix some issues reported like #476 #482 and possibly #519.
We were implementing partytown to run our GTM scripts though the web worker, but depending on the environment was working, for example in the pre-prod environment GA4 events are sent but in the prod environment there is no event sent to GA4, debugging the gtag.js script we detected that on production the GA4 scripts was failing ( silently ) , any of you having issues with GA4 can set a breakpoint here in the gtag.js here:

 catch ($I) {
            a.s.onFailure()
        }

The error , on our case, is:

TypeError: Failed to construct 'URL': Invalid URL
at E.get (126814e7-b6fb-4aa6-99f0-bae5f303fc6e:2:13380)

So, digging a bit deeper we detected the issue was coming from this part of the gtag.js script:

Hn = function(a) {
        var b = E.createElement("a");
        a && (b.href = a);
        var c = b.pathname;
        "/" !== c[0] && (a || Ab("TAGGING", 1),
        c = "/" + c);
        var d = b.hostname.replace(An, "");
        return {
            href: b.href,
            protocol: b.protocol,
            host: b.host,
            hostname: d,
            pathname: c,
            search: b.search,
            hash: b.hash,
            port: b.port
        }

So would be great to have this MR merged the sooner the better !
Thank you @franpeza !

@gioboa
Copy link
Member

gioboa commented Dec 29, 2023

Thanks @franpeza for this PR
@SirJalias 👏 you did a great recap

@gioboa
Copy link
Member

gioboa commented Dec 29, 2023

@SirJalias did you figure out why gtag is adding an empty anchor ?

@franpeza
Copy link
Contributor Author

We tried to debug the code but it was so difficult to follow the execution since it's minified, so we are not sure actually 🥲

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.

Thanks for your amazing help @franpeza

@gioboa gioboa changed the title fix: error thrown when trying to get properties from an anchor withou… fix: solve error with anchor without href Dec 29, 2023
@gioboa gioboa merged commit 3e9c3d1 into QwikDev:main Dec 29, 2023
8 checks passed
@franpeza franpeza deleted the empty-anchor branch December 31, 2023 16:34
@franpeza
Copy link
Contributor Author

franpeza commented Jan 8, 2024

hello @gioboa and thanks for merging this PR. Do you have an estimate of when there will be a new version that includes these changes? Thanks :)

@franpeza
Copy link
Contributor Author

franpeza commented Jan 8, 2024

by the way, and after digging into the open issues, this should solve #189 too

@gioboa
Copy link
Member

gioboa commented Jan 8, 2024

by the way, and after digging into the open issues, this should solve #189 too

Thanks 😊

hello @gioboa and thanks for merging this PR. Do you have an estimate of when there will be a new version that includes these changes? Thanks :)

I'll ping the core team, we have few improvements to publish 🚀

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 this pull request may close these issues.

3 participants