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

document.implementation.createHTMLDocument cicumvents patchDocument #174

Closed
quickshiftin opened this issue May 9, 2022 · 3 comments
Closed
Labels
help wanted Extra attention is needed needs reproduction Needs more information and a way to reproduce the issue

Comments

@quickshiftin
Copy link
Contributor

I'm trying to load Full Story via Partytown. I'm using the partytown repo and following the Local Development setup to test the integration in isolation as it is not a currently supported script.

The Full Story script is fetched via a reverse proxy, then when it starts being executed via Partytown, something blows up.

TypeError: Cannot set properties of undefined (setting 'id')
    at new e (fs.js:formatted:2601:68)
    at Object.302 (fs.js:formatted:2597:30)
    at n (fs.js:formatted:12491:28)
    at eval (fs.js:formatted:12515:13)
    at Proxy.eval (fs.js:formatted:12516:10)
    at run (partytown-ww-sw.js?v=0.5.4-dev1651873848564:859:37)
    at partytown-ww-sw.js?v=0.5.4-dev1651873848564:1879:33

Drilling into the error, it looks like Full Story is trying to create an element on Document object they've created. Essentially these two lines of code:

var e = document.implementation.createHTMLDocument("");
(t = e.createElement("base")).id

The second line fails, because createElement, which is an instance of WorkerBase returns undefined. Digging in a bit it looks like the value supplied for winId from the createElement function, (const winId = this[WinIdKey];) doesn't map to an environment. So when getOrCreateNodInstance tries to lookup the environment it fails to do so.

I'll plan to try to further isolate this, but that's what I have found so far. Any help or clues would be appreciated. I'm also attaching the debugging output in case that is helpful.

partytown-fullstory-debug

@quickshiftin quickshiftin changed the title Unknown issue running Full Story via Partytown document.implementation.createHTMLDocument cicumvents patchDocument May 10, 2022
@quickshiftin
Copy link
Contributor Author

quickshiftin commented May 10, 2022

I got it down to some trivial reproducible code:

// This works
b = document.createElement("base") // b is an instance of WorkerBase
console.log('base element?', b)

// This is broken
var e = document.implementation.createHTMLDocument("");
t = e.createElement("base") // t is undefined
console.log('base element?', t)

Could we potentially be missing a call to createEnvironment in the createHTMLDocument implementation?

@steve8708
Copy link
Contributor

hey @quickshiftin - we've just created an issue template, could you update your issue here to provide all the information the template asks for? it'll help us look into your issue https://github.com/BuilderIO/partytown/issues/new?assignees=&labels=&template=bug_report.md&title=

pasting below to make things easier:


Describe the bug
A clear and concise description of what the bug is.

To Reproduce
Steps to reproduce the behavior:

  1. Go to '...'
  2. Click on '....'
  3. Scroll down to '....'
  4. See error

Reproduction link
Please include a link to a Stackblitz or Codesandbox reproducing the issue. We will need to see the issue reproduced with hand-written code - we can't debug giant minified third party scripts directly. If you do not include a clean and simple reproduction of your issue, we won't be able to look into it until you do.

Expected behavior
A clear and concise description of what you expected to happen.

Screenshots
If applicable, add screenshots to help explain your problem.

Additional context
Add any other context about the problem here.

@steve8708 steve8708 added needs reproduction Needs more information and a way to reproduce the issue help wanted Extra attention is needed labels May 28, 2022
@adamdbradley
Copy link
Contributor

Fixed in #177

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed needs reproduction Needs more information and a way to reproduce the issue
Projects
None yet
Development

No branches or pull requests

3 participants