Refactor DOM attribute code (take two) #11815
Merged
Conversation
This file contains 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
gaearon
force-pushed
the
dom-injection-3
branch
4 times, most recently
from
573a695
to
a1f356a
Compare
|
The latest bench looks neutral to me. I added refactoring of the whitelists, inspired by #11733 but without the slowness.
|
gaearon
force-pushed
the
dom-injection-3
branch
2 times, most recently
from
5275a65
to
97c3708
Compare
I noticed some patterns weren't being tested.
The branching before the call is unnecessary because setValueForProperty() already has an internal branch that delegates to deleteValueForProperty() for null and undefined through the shouldIgnoreValue() check. The goal is to start unifying these methods because their separation doesn't reflect the current behavior (e.g. for unknown properties) anymore, and obscures what actually happens with different inputs.
Now we don't read propertyInfo twice in this case. I also dropped a few early returns. I added them a while ago when we had Stack-only tracking of DOM operations, and some operations were being counted twice because of how this code is structured. This isn't a problem anymore (both because we don't track operations, and because I've just inlined this method call).
The special cases for null and undefined already exist in setValueForAttribute().
Their naming is pretty confusing by now. For example setValueForProperty() calls setValueForAttribute() when shouldSetAttribute() is false (!). I want to refactor (as in, inline and then maybe factor it out differently) the relation between them. For now, I'm consolidating the callers to use setValueForProperty().
The naming of these methods is still very vague and conflicting in some cases. Will need further work.
This makes the flow clearer in my opinion.
It was previously duplicated. It's also suspiciously similar in purpose to shouldTreatAttributeValueAsNull() so I want to see if there is a way to unify them.
Its naming was confusing and it was used all over the place instead of more specific checks. Now that we only have one call site, we might as well inline and get rid of it.
They weren't being properly skipped because of the early return. I added tests for this case.
I think this PR looks worse on benchmarks because we have to read propertyInfo in different places. Originally I tried to get rid of propertyInfo, but looks like it's important for performance after all. So now I'm going into the opposite direction, and precompute propertyInfo as early as possible, and then just pass it around. This way we can avoid extra lookups but keep functions nice and modular.
It always exists because this function is only called for known properties.
I wrote this and then got confused myself.
Since we run these checks for all booleans, might as well remember it.
The changes reflect that SSR doesn't crash with symbols anymore (and just warns, consistently with the client).
Instead of using flags, explicitly group similar attributes/properties.
gaearon
force-pushed
the
dom-injection-3
branch
from
97c3708
to
3e0090b
Compare
gaearon
force-pushed
the
dom-injection-3
branch
from
7728729
to
12a1bac
Compare
gaearon
force-pushed
the
dom-injection-3
branch
from
ba16375
to
17f7ac9
Compare
|
This combines #11804 and #11733 which were both approved. In any case fixing bugs should be easier with the new test and Flow coverage I added. I also think this is easier to follow subjectively as attributes are now split into groups according to how they work. |
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.
Re-submit of #11804 which I reverted.
There's still a bug here.shouldTreatAsNullis the wrong way to think about it because this causes booleans that default totrueto becomefalsewhereas our intention is to ignore them for functions/symbols.I'll need to look at this again and figure out how to fix this.Mostly, this is a refactoring, but it does fix some edge cases where SSR used to crash (such as with Symbols) but now warns, consistently with the client.