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

Move hooks into Window instance #402

Merged
merged 11 commits into from
Aug 12, 2024
Merged

Conversation

olavoasantos
Copy link
Contributor

@olavoasantos olavoasantos commented Aug 2, 2024

Summary

The current implementation of the DOM polyfill uses an in memory singleton to manage internal hooks. This results in coupling of the polyfill with the global scope, preventing multiple instances of Window from being created without interfering with each other.

This PR refactors the management of these hooks by scoping them in the Window instance, decoupling the implementation from the global scope.

@olavoasantos olavoasantos self-assigned this Aug 2, 2024
@olavoasantos olavoasantos marked this pull request as ready for review August 2, 2024 18:43
@olavoasantos olavoasantos force-pushed the refactor/hooks-into-window-object branch from a008cd2 to 6989327 Compare August 8, 2024 21:12
Copy link
Member

@lemonmade lemonmade left a comment

Choose a reason for hiding this comment

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

I like this change a lot, thanks for putting it together. I left a few comments but I think this is quite close 👍

.changeset/chilled-phones-explain.md Outdated Show resolved Hide resolved
if (this[OWNER_DOCUMENT]) {
this[OWNER_DOCUMENT].defaultView[HOOKS].setText?.(this as any, str);
} else {
setTimeout(() =>
Copy link
Member

Choose a reason for hiding this comment

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

This is an issue because of us calling setData in the constructor, right? Instead of doing this, could we just assign to this[DATA] directly in the constructor (applying the same stringifying logic implemented above), which avoids this triggering a hook call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered that but that would skip the initial hook call and change the existing behaviour. I'm ok with that but it's a change in the existing behaviour. Alternatively, we can manually set the value via this[DATA] and schedule the hook call within the constructor.

Copy link
Member

Choose a reason for hiding this comment

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

I am OK to break this behavior. I think the library is actually broken in a different way here — we expose a createText hook

createText(text: Text, data: string): void;
, which should be used to cover the "initial value of a text node" case, but we don't seem to call that hook anywhere 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops haha Should I add setTimeout(() => this[HOOKS]?.createText ...) here then?

packages/polyfill/source/NamedNodeMap.ts Outdated Show resolved Hide resolved
packages/polyfill/source/EventTarget.ts Show resolved Hide resolved
@olavoasantos olavoasantos force-pushed the refactor/hooks-into-window-object branch from df0c6de to 1faebf7 Compare August 9, 2024 19:46
@olavoasantos olavoasantos requested a review from lemonmade August 9, 2024 20:09
Copy link
Member

@lemonmade lemonmade left a comment

Choose a reason for hiding this comment

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

Looks great, thanks so much for this improvement Olavo!

@lemonmade lemonmade merged commit 218ba3b into main Aug 12, 2024
5 checks passed
@lemonmade lemonmade deleted the refactor/hooks-into-window-object branch August 12, 2024 19:18
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