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

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

Open
wants to merge 1 commit into
base: master
from

Conversation

@enapupe
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

@enapupe enapupe force-pushed the enapupe:dom-setacessor-setAttribute-if-string branch from 37e28d2 to 69bd4bd Jan 17, 2017
@coveralls
Copy link

coveralls commented Jan 17, 2017

Coverage Status

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

@coveralls
Copy link

coveralls commented Jan 17, 2017

Coverage Status

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

@coveralls
Copy link

coveralls commented Jan 17, 2017

Coverage Status

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

@@ -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) {

This comment has been minimized.

Copy link
@developit

developit Jan 17, 2017

Member

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?

This comment has been minimized.

Copy link
@enapupe

enapupe Jan 17, 2017

Author

Done. Tests

@enapupe enapupe force-pushed the enapupe:dom-setacessor-setAttribute-if-string branch from 69bd4bd to d884a2a Jan 17, 2017
@coveralls
Copy link

coveralls commented Jan 17, 2017

Coverage Status

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

@developit
Copy link
Member

developit commented Jan 17, 2017

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

@enapupe enapupe force-pushed the enapupe:dom-setacessor-setAttribute-if-string branch from d884a2a to a5c83c4 Jan 18, 2017
@coveralls
Copy link

coveralls commented Jan 18, 2017

Coverage Status

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

@enapupe
Copy link
Author

enapupe commented Jan 18, 2017

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

@developit
Copy link
Member

developit commented Jan 18, 2017

@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".

@robdodson
Copy link

robdodson commented Jul 16, 2017

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!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.