Skip to content

check for typeof attribute value, if is string use setAttribute #511

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

check for typeof attribute value, if is string use setAttribute #511

wants to merge 1 commit into from

Conversation

Image for: Conversation
Copy link

enapupe commented Jan 17, 2017

I wasn't able to run react tests against this change but I tried to change the logic as minimum as possible.

ref #492
Copy link

Coverage increased (+0.008%) to 97.715% when pulling 69bd4bd on enapupe:dom-setacessor-setAttribute-if-string into 99ad1f3 on developit:master.
Copy link

Coverage increased (+0.008%) to 97.715% when pulling 69bd4bd on enapupe:dom-setacessor-setAttribute-if-string into 99ad1f3 on developit:master.
Copy link

Coverage increased (+0.008%) to 97.715% when pulling 69bd4bd on enapupe:dom-setacessor-setAttribute-if-string into 99ad1f3 on developit:master.
developit reviewed Jan 17, 2017
View reviewed changes
src/dom/index.js
@@ -64,7 +64,10 @@ export function setAccessor(node, name, old, value, isSvg) {
}
l[name] = value;
}
else if (name!=='list' && name!=='type' && !isSvg && name in node) {
else if (typeof value === 'string' && !isSvg && name in node) {
Copy link
Member

Choose a reason for hiding this comment

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

Think it'd be possible to invert this conditional and have String fall through to the complete attribute add/update/remove flow on line 75?
Copy link
Author

Choose a reason for hiding this comment

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

Done. Tests ✅
Copy link

Coverage remained the same at 97.707% when pulling d884a2a on enapupe:dom-setacessor-setAttribute-if-string into 99ad1f3 on developit:master.
Copy link
Member

Looking at the travis build, it actually seems like this might be better for performance! (not sure how but I'll take it)
Copy link

Coverage remained the same at 97.707% when pulling a5c83c4 on enapupe:dom-setacessor-setAttribute-if-string into dfa5912 on developit:master.
Copy link
Author

enapupe commented Jan 18, 2017

Don't forget to consider merging #495 after fixing this one.
Copy link
Member

@enapupe yup yup, I'll pair up the two. These are likely to go into 8.0 as the behavior probably constitutes a breaking change, or at least it has a chance to be "breaking".
developit added this to the 8.0 milestone Jan 18, 2017
developit mentioned this pull request Apr 2, 2017
Closed
developit removed this from the 8.0 milestone Apr 6, 2017
robdodson mentioned this pull request Jul 13, 2017
Open
Copy link

I was chatting with @developit about this PR today and he suggested adding a check to see if the value being set is an object and if so, falling back to using a property setter. I tried to outline the expected behavior in this comment (sorry it's a bit long!)
Copy link
Member

Since this is opened against v8, and the original issue (#492) has been closed, I'm going to close this PR. If there is a another situation where this change would be beneficial in Preact X, open a new issue with that case, and we'll take a look.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion feedback needed help wanted in X
6 participants