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

Partytown worker failing to initialize after passing non-serializable objects to postMessage #458

Merged
merged 6 commits into from
Sep 6, 2023

Conversation

GeorgiZhelev
Copy link
Contributor

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests

Description

After adding an example Klaviyo script tag, the Partytown workers fails to initialize because of the following error:

image

Further debugging shows that the Klaviyo object (window.klaviyo) is the culprit of the non-serializable objects (below links are images):
Analyzing Klaviyo object serializability
Debug logs before initial postMessage call with the results of readMainPlatform

More information at the following Loom recording

In the Loom there seems to be weird behavior with reloading with/without cache and with the "Update on reload" option checked for the service worker.

Gists mentioned in recording:
Gist 1
Gist 2

Debugging

The debugging I did was on a different version of Partytown, but:

  • the changelog doesn't indicate that this would matter in this case
  • the bug is identical across versions (reproduced on latest version)

NB: I tried adding a check whether an object is serializable before pushing it to the interfaces array, which helped with initializing the worker, but it then started throwing errors about missing constructors (I remember it missing a constructor for Element), which made me think the solution might not be a simple serializability check.

I can reproduce the debugging if that would help, but it's mostly just a isSerializable function and some console logs in readOwnImplementation.

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

@vercel
Copy link

vercel bot commented Sep 4, 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 Sep 6, 2023 0:38am

@mhevery
Copy link
Contributor

mhevery commented Sep 5, 2023

It looks like you have created a failing test case for us. ❤️ This is great! Any luck figuring out what the issue was? Not sure when I will get time to look into it, so if you can get a head start that would be great!

@GeorgiZhelev
Copy link
Contributor Author

Any luck figuring out what the issue was?

Unfortunately I don't understand enough of the context to fully understand what the interfaces array in the initWebWorkerData does but it seems like when Klaviyo is added, it contains non-serializable members that make the initial postMessage error due to failing to clone those members. To be clear, that's this error:
image

In my latest commit I've added the isSerializable function I've been using to test for serializability + some code to filter the non-serializable members from the interfaces array in readMainPlatform. That results in this error - something to do with the service worker expecting certain constructors to be available. It seems like simply checking for serializability might be removing too much information.

image

@GeorgiZhelev
Copy link
Contributor Author

GeorgiZhelev commented Sep 5, 2023

Any luck figuring out what the issue was?

After writing my previous comment, I realized that simply filtering for non-serializability might be too broad, as it would remove some objects entirely simply for having a non-serializable member that could instead be removed. I've added some code that aims to do the non-serializable member removal instead but that doesn't seem to work either.

Later edit: I might be calling the function on the wrong level of nested arrays, but either way - getting to the constructor error might be indicating that I'm misunderstanding the issue and this isn't on the correct path to a fix either way :)

P.S. Sorry for the code duplication, I don't view any of this as production viable code, just aiming to provide some extra debug info!

@mhevery mhevery merged commit e7eed80 into BuilderIO:main Sep 6, 2023
6 checks passed
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