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

hyperscript: Revert attrs.class creation logic to what we had in v1.0.1. fix #1764 #1769

Merged
merged 2 commits into from Apr 4, 2017

Conversation

Projects
None yet
4 participants
@pygy
Copy link
Member

pygy commented Apr 3, 2017

@tivac Once this is merged, I'd like to release v1.1.1 ASAP, but I'd rather have you around if anything goes wrong...

@pygy pygy referenced this pull request Apr 3, 2017

Closed

Q: null as class name #1764

@pygy pygy added this to the v1.1.1 milestone Apr 3, 2017

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Apr 3, 2017

Sorry but I'm on vacation until the 6th still. It'll be fine, just follow the instructions! 😊

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Apr 3, 2017

@tivac Enjoy your time off :-)

@isiahmeadows Are you ok with this?

@pygy pygy requested a review from isiahmeadows Apr 3, 2017

@lhorie

This comment has been minimized.

Copy link
Member

lhorie commented Apr 3, 2017

My only worry here is that delete deopts

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Apr 3, 2017

@lhorie Nice to see you around :-)

What about keeping either class or className then? updateAttrs() needed some work as well...

@pygy pygy removed the request for review from isiahmeadows Apr 3, 2017

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Apr 3, 2017

@pygy Sorry for the delayed response, but LGTM as it stands now. BTW, @lhorie's right about the delete - we'd prefer to not kick user-provided attributes to dictionary mode, since that'll affect not only our perf, but theirs as well. Dictionary mode is great when you're using an object as a mutable hash map with frequent key updates, but not with just frequent reads (which is our case with attributes).

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Apr 3, 2017

I myself prefer keeping whatever the user provided, since it's more intuitive. So that works.

@isiahmeadows
Copy link
Collaborator

isiahmeadows left a comment

Found a few nits, but that's it.

attrs.className = state.attrs.className + " " + className
}
}
if (className != null || state.attrs.className != null) attrs[classAttr] = state.attrs.className == null ? className || "" : state.attrs.className + (className ? " " + className : "")

This comment has been minimized.

@isiahmeadows

isiahmeadows Apr 3, 2017

Collaborator

Could you split this apart to remove some redundant branching and make the logic clearer?

if (state.attrs.className != null) {
  attrs[classAttr] = state.attrs.className + (className ? " " + className : "")
} else if (className != null) {
  attrs[classAttr] = className || ""
}
@@ -62,7 +54,7 @@ function execSelector(state, attrs, children) {
}

function hyperscript(selector) {
// Because sloppy mode sucks
// IE9 doesn't support strict mode

This comment has been minimized.

@isiahmeadows

isiahmeadows Apr 3, 2017

Collaborator

👍 for improving the comment.

@@ -512,7 +512,7 @@ module.exports = function($window) {
}
if (old != null) {
for (var key in old) {
if (attrs == null || !(key in attrs)) {
if (attrs == null || !(key in attrs || key === "className" && ("class" in attrs) || key === "class" && ("className" in attrs))) {

This comment has been minimized.

@isiahmeadows

isiahmeadows Apr 3, 2017

Collaborator

Both occurrences of "<x>" in attrs don't need the parentheses.


render(root, [a]);

o(a.dom.attributes["class"].nodeValue).equals("test")

This comment has been minimized.

@isiahmeadows

isiahmeadows Apr 3, 2017

Collaborator

You don't need to quote these - IE9 won't choke over them, and some of the member accesses already use .class.

This comment has been minimized.

@pygy

pygy Apr 3, 2017

Member

I copied/pasted other tests. The whole file is like this...

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Apr 3, 2017

@isiahmeadows I would have expected attrs to be highly polymorphic and thus always treated as dictionaries by the engines... We were also adding a className key in many cases, trashing the hidden class...

👍 for the suggested changes.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Apr 3, 2017

@pygy

I would have expected attrs to be highly polymorphic and thus always treated as dictionaries by the engines... We were also adding a className key in many cases, trashing the hidden class...

Actually, simple hidden class updates are relatively cheap unless you're in a super hot loop. But the main reason I wanted to avoid delete is so we can avoid creating negative performance implications in user code, keeping the megamorphic property access limited to us.

Besides, V8 is fairly unique in optimizing deep, static property accesses so aggressively. Firefox and Edge barely take a hit with dictionary mode.

@pygy pygy force-pushed the pygy:fix-class-null branch 3 times, most recently from 1e1e9d4 to a5068a3 Apr 3, 2017

if (state.attrs.className != null) {
attrs[classAttr] = state.attrs.className + (className != null ? " " + className : "")
} else if (className != null) {
attrs[classAttr] = className

This comment has been minimized.

@pygy

pygy Apr 3, 2017

Member

@isiahmeadows I've slightly changed your suggestion so that only null and undefined are treated as an absence of value. The other falsy values will either be stringified here or by setAttribute().

Edit: looking back, those were my bugs ;-)

@isiahmeadows
Copy link
Collaborator

isiahmeadows left a comment

LGTM.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Apr 4, 2017

Digging deeper still into this, it's a mess :-)

http://jsbin.com/witofosemi/1/edit?html,js,console

And it is just for the first render, ignoring issues on update and with the recycling pool, which is neutered by rendering [] twice before each trial.

So, for v1.1.1, I'll go with a fix that's bug-for-bug compatible with v1.0 and we'll revisit this later.

@pygy pygy force-pushed the pygy:fix-class-null branch from a5068a3 to 3413b4b Apr 4, 2017

@pygy pygy force-pushed the pygy:fix-class-null branch from 3413b4b to 7d019e7 Apr 4, 2017

@pygy pygy force-pushed the pygy:fix-class-null branch from 7d019e7 to 98e3cbd Apr 4, 2017

@pygy pygy changed the title hyperscript: cleanup `attrs.class` when setting `attrs.className`. fix #1764 hyperscript: Revert attrs.class creation logic to what we had in v1.0.1. fix #1764 Apr 4, 2017

@pygy pygy merged commit d60e7ab into MithrilJS:next Apr 4, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Apr 4, 2017

@pygy That's disheartening..

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Apr 4, 2017

@isiahmeadows on the bright side, v1.1.1 went out without a hitch!

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Apr 4, 2017

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Apr 4, 2017

@isiahmeadows BTW, in case you didn't check the diff, I kept your selector compiler optimizations, I only reverted to the old output by changing a couple of loose null checks to strict undefined ones.

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Apr 4, 2017

If nothing else I'm very happy that the release automation worked without me having to hand-hold it!

@pygy was the documentation of the process ok?

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Apr 4, 2017

@tivac mostly, yes... I'd add a "Switch to master and merge next" step in the explanations. I wasn't sure where to start from by just reading the docs. Otherwise it is magical :-)

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Apr 5, 2017

@pygy My comment was about something else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment