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: fix #1773, fix #2172 #2174

Merged
merged 6 commits into from Jun 7, 2018

Conversation

Projects
None yet
3 participants
@pygy
Copy link
Member

pygy commented Jun 2, 2018

This makes the attrs take precedence over the selector (#2172), and improves class normalization (#1773).

The docs and change log will follow here.

Description

See the respective issues for a detailed discussion.

m("[a=b]", {a: "c"}) will now have a set to "c", and m("[a=b]", {a: null}) will have it set to null (likewise for undefined).

regarding classes:

  • m(".foo", {}).attrs will be {className: "foo"}
  • m(".foo", {className: "bar"}).attrs will be {className: "foo bar"}
  • m(".foo", {class: "bar"}).attrs will be {class: "foo bar"} (we re-use the class field if users chose it. Leo said it was slower, but I didn't see any difference in the benchmark)
  • m(".foo", {class: false}), m(".foo", {class: null}) and m(".foo", {class: undefined}) all behave as if there was no class in the attrs.

How Has This Been Tested?

The test suite was adapted,

The benchmarks (improved a bit to cover class merging) were run in Chrome, Safari and Firefox (desktop). No difference was noticed (though the variance from run to run was high even though the standard error on the mean was always ~1% or 2%).

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation change

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated docs/change-log.md

@pygy pygy requested a review from tivac as a code owner Jun 2, 2018

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Jun 3, 2018

Actually, re-using the class field introduces a bug when diffing m('.foo', {class: "bar"}) with m(".foo"), i.e.

{tag: "div", attrs: {class: "foo bar"}} diffed with {tag: "div", attrs: {className: "foo"}}.

It will first set the className to "foo", then notice the absence of class and remove it.

There are two possible solutions:

  • normalize to className as we were doing, leaving an undefined class field (or deleting it, but it may trigger deopts according to Leo).
  • do some magic in updateAttrs, but those loops are pretty hot, I'm afraid anything extra happening in them would cause perf problems...
@barneycarroll

This comment has been minimized.

Copy link
Member

barneycarroll commented Jun 6, 2018

Nice work @pygy

@pygy pygy force-pushed the pygy:hyperscript-fix-1773-2172-2018-06 branch from 580f162 to 43d8afe Jun 6, 2018

pygy added some commits Jun 6, 2018

[performance] use individual files rather than the build, revamp the …
…attrs code to reduce variance, reset the scratch pad more reliably

@pygy pygy force-pushed the pygy:hyperscript-fix-1773-2172-2018-06 branch from 506a4a1 to ad07b35 Jun 6, 2018

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Jun 6, 2018

Thanks @barneycarroll. This still needs a docs update, then we're done.

@pygy pygy requested review from barneycarroll and spacejack Jun 7, 2018

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Jun 7, 2018

@tivac @barneycarroll @spacejack: I'd appreciate a review from either one of you.

The new integration tests suite is long and can be skipped mostly (it is a 9x10 matrix with little interest once you've read the first two nested spec (create nodes, and update from the first scenario)).

@tivac

tivac approved these changes Jun 7, 2018

Copy link
Member

tivac left a comment

👏🏻

@pygy pygy merged commit fed0846 into MithrilJS:next Jun 7, 2018

1 check passed

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

This comment has been minimized.

Copy link
Member

pygy commented Jun 7, 2018

Thanks @tivac

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