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

Do not normalise component children on ingestion #2155

Merged
merged 6 commits into from May 29, 2018

Conversation

Projects
None yet
4 participants
@barneycarroll
Copy link
Member

barneycarroll commented May 18, 2018

Description

This PR seeks to change vnode normalisation behaviour to raise a distinction between component vnode children and every other kinds of vnode children. Nominally, component children should only be fully normalised upon interpolation of the component view (or instance) - not upon component vnode normalisation. In a similar way to the fact that element vnodes render non-key, non-lifecycle attributes as events, DOM properties or DOM attributes but components do not (unless they are passed on as such in the component view), so component children should not be fully normalised unless called to do so by their being invoked in the view.

Motivation and Context

This enables more flexibility in developer experience when writing custom component interfaces by being less strict about the nature of children. Significantly, we resolve #2050 such that functions passed in as children can be accessed without having to query an interstitial structure which conforms neither to the input structure, nor to a rational vnode entity in its own right:

// Call site:
m(Component, () => 'Hello')

// Before:
const Component = {
  view: v => v.children[0].children()
  // Upon normalisation, `() => 'Hello'` is coerced into a text node as a last resort since it is none of
  // a nullish value (empty slot), an array (fragment), or an explicit complex vnode (element, component).
  // Its original value is therefore stored as the `children` attribute of the newly created text node, the first
  // item of the component's `children` (which is always an array)
}

// After:
const Component = {
  view: v => v.children[0]()
  // Hyperscript performs a necessary transformation to convert trailing arguments into an array,
  // but Vnode normalisation functions proper are not invoked 
}

How Has This Been Tested?

I modified all existing tests which made assumptions about component children and added a new one, bringing in render, vnode & hyperscript - to deal with component normalisation steps holistically.
https://github.com/MithrilJS/mithril.js/blob/deferred-component-children-normalization/render/tests/test-normalizeComponentChildren.js

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 added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated docs/change-log.md
  • Change has been implemented

@barneycarroll barneycarroll requested a review from pygy as a code owner May 18, 2018

@barneycarroll

This comment has been minimized.

Copy link
Member

barneycarroll commented May 18, 2018

I tried running the inbuilt perf tools on this but couldn't get any consistent comparisons: by running tests repeatedly and alternating I could get either branch 10% better than the other for all tests. The most consistent thing was that for any given run, all tests would be either faster or slower than their last run.

for (var i = 0; i < children.length; i++) {
children[i] = Vnode.normalize(children[i])
Vnode.normalizeChildren = function normalizeChildren(input) {
var children = []

This comment has been minimized.

@barneycarroll

barneycarroll May 18, 2018

Member

The issue with having non-normalized children and normalized instance children is that up until now, there was no need to distinguish between the two so they were effectively the same thing. New arrays here stops downstream code mutating upstream references, which is otherwise pretty unsettling.

This comment has been minimized.

@pygy

pygy May 18, 2018

Member

That's a good point, but I'm afraid it will not be good for performance.

vnodes should IMO be treated as owned by render once they leave user code.

OTOH having your children turn from ["a", "b", "c"] to an array of vnodes can definitely be surprising.

I'm on the fence on this...

This comment has been minimized.

@barneycarroll

barneycarroll May 18, 2018

Member

The performance concern jumps out. I couldn't get anything significant out of npm run perf — that is to say of the test suites included, the variation between any given pass and the next is too large to notice any difference plus or minus this patch. I'm tempted to think that in the grand scheme of things, creating disposable arrays is trivial but I'm at a loss as to how to validate it.

IMO your points about ownership & surprise are not contradictory. From a semantic perspective, this patch seeks to address the fact that components are an exception in hyperscript transformation inasmuch as children (and non-lifecycle attributes) have not left the user domain until they have been yielded back into render space by the component's internals.

@barneycarroll barneycarroll requested a review from tivac May 18, 2018

@pygy

This comment has been minimized.

Copy link
Member

pygy commented May 18, 2018

Great PR, thanks!

If you don't see any perf impact, I'd be tempted to merge it as is, with normalizeChildren returning a new array.

I just introduced a merge conflict by closing #2064, sorry

@pygy

pygy approved these changes May 18, 2018

@pygy

This comment has been minimized.

Copy link
Member

pygy commented May 18, 2018

Don't forget to update the change log (and while you're at it to credit @magikstm for #2064, I didn't do it because I didn't want to introduce a conflict with this... Having just read the title of this PR, I didn't think #2064 would be conflictual anyway :-))

@barneycarroll

This comment has been minimized.

Copy link
Member

barneycarroll commented May 18, 2018

@JAForbes I know you were distasteful of the idea of surface API sloppiness in allowing maybe-vnodes-maybe-allsorts , what do you reckon about this?

@JAForbes

This comment has been minimized.

Copy link
Contributor

JAForbes commented May 26, 2018

I see why you're taking this tack (lazy normalization). I think either way we should have that, it's a good change.

As for the intended use: I think I'd prefer first class support for render functions that get assigned to a different property instead of vnode.children. I know that is sort of beyond the scope of this PR though but if it's left to long could lead to some unhappy refactoring's later.

But I certainly don't think that discussion needs to be had here or hold up this great addition.

@barneycarroll barneycarroll merged commit 1579fe8 into next May 29, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Sep 1, 2018

The label here is tentative - this might be breaking, so take it with a grain of salt.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Sep 25, 2018

Not landing this in v1.2 (it's a lot more breaking with fresh eyes).

@isiahmeadows isiahmeadows deleted the deferred-component-children-normalization branch Nov 28, 2018

@isiahmeadows isiahmeadows restored the deferred-component-children-normalization branch Nov 28, 2018

@isiahmeadows isiahmeadows deleted the deferred-component-children-normalization branch Nov 28, 2018

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