feat: enable stringification of DOM nodes controlled by FAST's renderer #6485
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
EisenbergEffect
requested review from
chrisdholt,
janechu and
nicholasrice
as code owners
📊 Tachometer Benchmark ResultsSummaryclickTrigger10x
create10k
createDelete5x
runFile1k
update10th
usedJSHeapSize
Resultsobservable-runFile1k
runFile1k
usedJSHeapSize
render-create10k
create10k
usedJSHeapSize
render-createDelete5x
createDelete5x
usedJSHeapSize
render-update10th
update10th
usedJSHeapSize
repeat-basic-splice-itemCount=1000&deleteCount=20&addCount=20
clickTrigger10x
usedJSHeapSize
repeat-nested-push-itemCount=100&addCount=20
clickTrigger10x
usedJSHeapSize
repeat-nested-reverse-itemCount=100
clickTrigger10x
usedJSHeapSize
repeat-nested-shift-itemCount=100
clickTrigger10x
usedJSHeapSize
repeat-nested-unshift-itemCount=100&addCount=20
clickTrigger10x
usedJSHeapSize
|
EisenbergEffect
added
improvement
nicholasrice
approved these changes
Oct 28, 2022
Contributor
nicholasrice
left a comment
nicholasrice
left a comment
There was a problem hiding this comment.
Looks good, thanks Rob! Did you determine that $fastController wouldn't be an issue?
Contributor
Author
Oops. Forgot to check that one. That's likely an issue. I'm off for the weekend. I'll add some code for that on Monday. |
Contributor
Author
Ok, I've added that scenario. Should be good to go now. |
chrisdholt
approved these changes
Oct 31, 2022
EisenbergEffect
deleted the
users/eisenbergeffect/prevent-stringify-error
branch
9 tasks
janechu
pushed a commit
that referenced
this pull request
Jun 10, 2024
…er (#6485) * test: add basic ref tests * fix: prevent children behavior from causing errors if dom stringified * test: add slot stringify guard * fix: prevent all binding types from causing errors if dom stringified * doc: mark toJSON methods as internal so they don't clutter the APIs * Change files * chore: fix change file * fix: prevent serialized web components from throwing Co-authored-by: EisenbergEffect <roeisenb@microsoft.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request
📖 Description
Some 3rd party script have odd behaviors where they attempt to
JSON.stringifyDOM node instances. This can result in errors in some cases where FAST is storing controllers/state on the node for quick access. This PR prevents errors that could occur during stringification.🎫 Issues
👩💻 Reviewer Notes
The main fix was to provide explicit
toJSONimplementations on core FAST types. These implementations are no ops so that FAST types aren't serialized at all. The problem caused by this situation was a bit more complex than this and so also required a few other internal changes to core types to ensure that we could no op the quick access properties to begin with.📑 Test Plan
Tests were added for:
refslottedchildrenonChangebindingsoneTimebindingstwoWaybindingssignalbindingseventbindingsAdditionally, some basic tests were added for
refwhich were previously missing.✅ Checklist
General
$ yarn change⏭ Next Steps
This will create some merge conflicts with the security work branch. So, next steps will be to fix all of that up.
After the security work is merged we'll also need to do a general cleanup pass on
fast-element. While working on the last couple of PRs I've noticed some negative effects of all the recent code churn and they need to be addressed before this library is ready to be used in production.