Skip to content

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

Closed
enapupe wants to merge 1 commit into
preactjs:masterfrom
enapupe:dom-setacessor-setAttribute-if-string
Closed

check for typeof attribute value, if is string use setAttribute#511
enapupe wants to merge 1 commit into
preactjs:masterfrom
enapupe:dom-setacessor-setAttribute-if-string

Conversation

@enapupe

@enapupe enapupe commented Jan 17, 2017

Copy link
Copy Markdown

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

ref #492

@coveralls

Copy link
Copy Markdown

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
Copy Markdown

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
Copy Markdown

Coverage Status

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

Comment thread src/dom/index.js Outdated
l[name] = value;
}
else if (name!=='list' && name!=='type' && !isSvg && name in node) {
else if (typeof value === 'string' && !isSvg && name in node) {

Copy link
Copy Markdown
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
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done. Tests ✅

@coveralls

Copy link
Copy Markdown

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
Copy Markdown
Member

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

@coveralls

Copy link
Copy Markdown

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

enapupe commented Jan 18, 2017

Copy link
Copy Markdown
Author

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

@developit

Copy link
Copy Markdown
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".

@robdodson

Copy link
Copy Markdown

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

@andrewiggins

Copy link
Copy Markdown
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants