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

feat: improve forwarding-events with preserveBehavior #530

Merged
merged 4 commits into from
Jan 23, 2024

Conversation

franpeza
Copy link
Contributor

@franpeza franpeza commented Jan 8, 2024

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests

Description

This PR adds the ability to preserve the behavior of forwarded functions. This comes in candy for some scenarios, for example, some third parties integrations that monitor the global dataLayer array in order to work. When GA is handled by partytown the array will be empty, this new functionality allows to keep the events there.

Even if the motivation for this is GA and the dataLayer array, this would work for any other forwarded method, as it makes the trick including a call to the original method in the patched version (in fact, in the tests added there is a case where the forwarded method is unshift instead of push).

If you want to enable this new setting, you would do it like this:
forward: ['simpleProperty', ['dataLayer.push', { preserveBehavior: true }]],
I also contemplated the possibility of doing so:
forward: ['simpleProperty', { property: 'dataLayer.push', preserveBehavior: true }],
but I prefer the first way. Anyway I'm open to implement any other way of doing it.

Use cases and why

    1. GA integration, as described above.

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 (not yet)
  • Added new tests to cover the fix / functionality

If you think this PR is worth, I could add the documentation of the new setting.
Waiting for your feedback, thank you!

Copy link

vercel bot commented Jan 8, 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 Jan 10, 2024 0:28am

@gioboa
Copy link
Member

gioboa commented Jan 9, 2024

Thanks @franpeza I think this is a nice improvement. my only concern is to have to different way of doing the same thing.
Let's just keep the first one.
forward: ['simpleProperty', ['dataLayer.push', { preserveBehavior: true }]]
Are there any open issues for this?

@franpeza
Copy link
Contributor Author

thanks @gioboa, what I was trying to expose was some possible ways to allow to inform the property, but the one I implemented is precisely the one you mentioned (and my favourite one BTW) So that would be the only way of enabling this new setting.

Don't know if there's any open issue regarding this topic, not on my side at least.

I will try to make some time today or tomorrow to document the property with this shape:

forward: ['simpleProperty', ['dataLayer.push', { preserveBehavior: true }]]

@franpeza
Copy link
Contributor Author

there seems to be some kind of problem with pnpm running the prettier action

@gioboa
Copy link
Member

gioboa commented Jan 10, 2024

I'll fix it

--- UPDATE ---

✅ Fixed

@franpeza
Copy link
Contributor Author

thanks @gioboa

@gioboa
Copy link
Member

gioboa commented Jan 10, 2024

Tests are so unstable 😢 I can't figure out the why

@franpeza
Copy link
Contributor Author

franpeza commented Jan 10, 2024

Tests are so unstable 😢 I can't figure out the why

The most unstable seems to be the webkit one, yep

I just added some documentation about the new setting, please take a look if you can :)

@gioboa gioboa changed the title feat: Preserve behavior feat: improve forwarding-events with preserveBehavior Jan 23, 2024
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.

I double checked this PR with the core team and it looks awesome.
Thanks @franpeza
Let's merge it.

@gioboa gioboa merged commit 718b1a4 into QwikDev:main Jan 23, 2024
10 checks passed
@franpeza
Copy link
Contributor Author

Thanks for merging it!!

@franpeza franpeza deleted the preserve-behavior branch January 24, 2024 12:54
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.

2 participants