Refactor to public APIs for head/tail #54

Merged
merged 1 commit into from Jun 8, 2016

Conversation

Projects
None yet
5 participants
@mixonic
Contributor

mixonic commented Jun 3, 2016

In the template for ember-wormhole, use helpers that register an element to the parent wormhole component. The wormhole component can then use those elements to move to the target. This lessens the private APIs used by wormhole, but still uses private APIs to access the DOM or Fastboot SimpleDOM based on where the code is running. This could use all private API if we made SimpleDOM a dependency, but that would just inflate the JS payload with little benefit.

This unblocks Fastboot rendering. It will also mean dropping support for Ember versions before 1.13. See #47 for discussion.

TODO:

  • Remove support for Ember pre-1.13
  • Check for isDestroyed in the afterRender callback.
  • Document Fastboot status
  • Document/test passing a bare element to destinationElement
  • Move SimpleDOM/DOM compat tooling to a util module
  • Document subclassing of the wormhole component class skipping this, but after merge and release I will open an issue/PR on the x-favicon repo
addon/components/ember-wormhole.js
+ this._didInsert = true;
+ run.schedule('afterRender', () => {
+ // read the values from the ember-wormhole-placeholder helpers
+ this._firstNode = this.get('registry.head');

This comment has been minimized.

@krisselden

krisselden Jun 4, 2016

Contributor

what is the registry thing solving?

@krisselden

krisselden Jun 4, 2016

Contributor

what is the registry thing solving?

This comment has been minimized.

@krisselden

krisselden Jun 4, 2016

Contributor

Nevermind, it is because this is before children are rendered, is there no hook on didCreateElement? I thought there was, anyway, I don't think this should be generic and use slots. Not only because this isn't very JIT friendly but because it is confusing like it is some general feature.

@krisselden

krisselden Jun 4, 2016

Contributor

Nevermind, it is because this is before children are rendered, is there no hook on didCreateElement? I thought there was, anyway, I don't think this should be generic and use slots. Not only because this isn't very JIT friendly but because it is confusing like it is some general feature.

This comment has been minimized.

@mixonic

mixonic Jun 4, 2016

Contributor

didInsertElement is not fired for Fastboot. We kind of want a "fire all hooks I know what I'm doing" mode for Fastboot rendering. Doesn't seem likely. So instead we emulate the timing of didInsertElement using the hooks that do fire.

Regarding slots you suggest this should use an array instead? Would pre-defining the shape also be sufficient?

  init() {
    this._super(...arguments);
    this.wormholeNodeRegistry = { head: null, tail: null };
  },
@mixonic

mixonic Jun 4, 2016

Contributor

didInsertElement is not fired for Fastboot. We kind of want a "fire all hooks I know what I'm doing" mode for Fastboot rendering. Doesn't seem likely. So instead we emulate the timing of didInsertElement using the hooks that do fire.

Regarding slots you suggest this should use an array instead? Would pre-defining the shape also be sufficient?

  init() {
    this._super(...arguments);
    this.wormholeNodeRegistry = { head: null, tail: null };
  },

This comment has been minimized.

@mixonic

mixonic Jun 4, 2016

Contributor

cool, refactored away from this.

@mixonic

mixonic Jun 4, 2016

Contributor

cool, refactored away from this.

@@ -0,0 +1,3 @@
+{{ember-wormhole-placeholder registry=registry as="head" dom=dom}}

This comment has been minimized.

@krisselden

krisselden Jun 4, 2016

Contributor

Make a helper ember-wormhole-placeholder that returns the value of invoking its first param.

Instead of passing all the internals, pass a bound function

{{ember-wormhole-placeholder _createHead~}}
{{yield~}}
{{ember-wormhole-placeholder _createTail~}}
@krisselden

krisselden Jun 4, 2016

Contributor

Make a helper ember-wormhole-placeholder that returns the value of invoking its first param.

Instead of passing all the internals, pass a bound function

{{ember-wormhole-placeholder _createHead~}}
{{yield~}}
{{ember-wormhole-placeholder _createTail~}}

This comment has been minimized.

@mixonic

mixonic Jun 4, 2016

Contributor

Great idea- I'll actually pass the _firstNode and _lastNodes directly. Seems to be working well.

@mixonic

mixonic Jun 4, 2016

Contributor

Great idea- I'll actually pass the _firstNode and _lastNodes directly. Seems to be working well.

addon/components/ember-wormhole.js
+ // keep a ref to 'dom' and 'registry' because those will be passed to
+ // the ember-wormhole-placeholder helper
+ let dom = this.renderer._dom;
+ this.set('dom', dom);

This comment has been minimized.

@krisselden

krisselden Jun 4, 2016

Contributor

you don't need to use set here as this is internal and not something that changes

@krisselden

krisselden Jun 4, 2016

Contributor

you don't need to use set here as this is internal and not something that changes

This comment has been minimized.

@mixonic

mixonic Jun 4, 2016

Contributor

agreed.

@mixonic

mixonic Jun 4, 2016

Contributor

agreed.

addon/components/ember-wormhole.js
+ let dom = this.renderer._dom;
+ this.set('dom', dom);
+ this.set('registry', {});
+ this._didInsert = false;

This comment has been minimized.

@krisselden

krisselden Jun 4, 2016

Contributor

instead of the registry do:

this._head = this._tail = undefined;
this._createHead = () => {
  return this._head = dom.createTextNode("");
};
this._createTail = () => {
   return this._tail = dom.createTextNode("");
};
@krisselden

krisselden Jun 4, 2016

Contributor

instead of the registry do:

this._head = this._tail = undefined;
this._createHead = () => {
  return this._head = dom.createTextNode("");
};
this._createTail = () => {
   return this._tail = dom.createTextNode("");
};

This comment has been minimized.

@krisselden

krisselden Jun 4, 2016

Contributor

the helper is just:

helper(([factory]) => factory());
@krisselden

krisselden Jun 4, 2016

Contributor

the helper is just:

helper(([factory]) => factory());

This comment has been minimized.

@krisselden

krisselden Jun 4, 2016

Contributor

this is optimized in glimmer 2 as well if you just do:

{{ember-wormhole-placeholder (unbound _createHead)}}

This will only be evaluated at render time, this is nice because glimmer 2 won't try to track bounds for update (since you are moving the node) it will be treated as static.

@krisselden

krisselden Jun 4, 2016

Contributor

this is optimized in glimmer 2 as well if you just do:

{{ember-wormhole-placeholder (unbound _createHead)}}

This will only be evaluated at render time, this is nice because glimmer 2 won't try to track bounds for update (since you are moving the node) it will be treated as static.

This comment has been minimized.

@mixonic

mixonic Jun 4, 2016

Contributor

👍

@mixonic

mixonic Jun 4, 2016

Contributor

👍

addon/components/ember-wormhole.js
export default Ember.Component.extend({
+ layout,
to: computed.alias('destinationElementId'),
destinationElementId: null,
destinationElement: computed('destinationElementId', 'renderInPlace', function() {

This comment has been minimized.

@krisselden

krisselden Jun 4, 2016

Contributor

I know this is refactoring something already here, but this should not be a CP, we should just have a findDestination when we need to update the destination. The component hooks will be invoked if the input has changed. This isn't a property that is actually bound to anything outside of the component itself. If the hooks are invoked for didUpdate, you can compare if your head's parentNode matches the new location.

@krisselden

krisselden Jun 4, 2016

Contributor

I know this is refactoring something already here, but this should not be a CP, we should just have a findDestination when we need to update the destination. The component hooks will be invoked if the input has changed. This isn't a property that is actually bound to anything outside of the component itself. If the hooks are invoked for didUpdate, you can compare if your head's parentNode matches the new location.

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Jun 4, 2016

Contributor

Another pass at the implementation here for review. @krisselden I did not refactor destinationElement away from being a CP. Can we punt that and the refactor away from observers to another pass?

The ember-try config here is updated, perhaps travis needs a cache flush.

I'll flesh out the docs changes later today.

Contributor

mixonic commented Jun 4, 2016

Another pass at the implementation here for review. @krisselden I did not refactor destinationElement away from being a CP. Can we punt that and the refactor away from observers to another pass?

The ember-try config here is updated, perhaps travis needs a cache flush.

I'll flesh out the docs changes later today.

@chrislopresto

This comment has been minimized.

Show comment
Hide comment
@chrislopresto

chrislopresto Jun 4, 2016

Contributor

@mixonic I cleared the Travis cache and rekicked the build.

Contributor

chrislopresto commented Jun 4, 2016

@mixonic I cleared the Travis cache and rekicked the build.

@chrislopresto

This comment has been minimized.

Show comment
Hide comment
@chrislopresto

chrislopresto Jun 4, 2016

Contributor

@mixonic Ah, the travis.yml needs to be updated.

Contributor

chrislopresto commented Jun 4, 2016

@mixonic Ah, the travis.yml needs to be updated.

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Jun 6, 2016

Contributor

This is pretty wrapped up- @bantic can you please confirm with your fastboot demo app that this PR functions correctly?

I'm actually quite curious what will happen upon initial re-render of the page. The idea for fastboot users might be that the nodes inside that part of the page are destroyed and re-rendered. But we should know what is happening for sure.

Contributor

mixonic commented Jun 6, 2016

This is pretty wrapped up- @bantic can you please confirm with your fastboot demo app that this PR functions correctly?

I'm actually quite curious what will happen upon initial re-render of the page. The idea for fastboot users might be that the nodes inside that part of the page are destroyed and re-rendered. But we should know what is happening for sure.

@mixonic mixonic referenced this pull request in ember-fastboot/ember-cli-fastboot Jun 6, 2016

Closed

Should fastboot execute code scheduled for 'afterRender' queue? #207

@bantic

This comment has been minimized.

Show comment
Hide comment
@bantic

bantic Jun 7, 2016

Contributor

@mixonic Yeah, this works great with the demo app I was testing.

Contributor

bantic commented Jun 7, 2016

@mixonic Yeah, this works great with the demo app I was testing.

@lukemelia

This comment has been minimized.

Show comment
Hide comment
@lukemelia

lukemelia Jun 7, 2016

Contributor

Nice work on this, all. Thanks for driving the implementation@mixonic! I'm +1 on merging and releasing 0.4.0 assuming @chrislopresto is as well.

Contributor

lukemelia commented Jun 7, 2016

Nice work on this, all. Thanks for driving the implementation@mixonic! I'm +1 on merging and releasing 0.4.0 assuming @chrislopresto is as well.

@bantic

This comment has been minimized.

Show comment
Hide comment
@bantic

bantic Jun 7, 2016

Contributor
Contributor

bantic commented Jun 7, 2016

@@ -0,0 +1,3 @@
+{{ember-wormhole-placeholder (unbound _firstNode) ~}}
+{{yield ~}}
+{{ember-wormhole-placeholder (unbound _lastNode) ~}}

This comment has been minimized.

@chrislopresto

chrislopresto Jun 8, 2016

Contributor

@mixonic With the disclaimer that I haven't fastboot-ed, can you explain how this differs from:

{{unbound _firstNode ~}}
{{yield ~}}
{{unbound _lastNode ~}}

If these approaches are functionally equivalent, we could ditch the placeholder helper (and rename _firstNode and _lastNode to something more expressive).

@chrislopresto

chrislopresto Jun 8, 2016

Contributor

@mixonic With the disclaimer that I haven't fastboot-ed, can you explain how this differs from:

{{unbound _firstNode ~}}
{{yield ~}}
{{unbound _lastNode ~}}

If these approaches are functionally equivalent, we could ditch the placeholder helper (and rename _firstNode and _lastNode to something more expressive).

Refactor to public APIs for Fastboot compatibility
Generate placeholder nodes in the `ember-wormhole` component and use the
`unbound` helper in its template to place those nodes before and after
userland yielded content. The wormhole component then uses these
placeholders to move its contents to the target element.

Also adds public documentation that `destinationElement` can be passed
directly instead of `to` with a string element ID.

 * [breaking] Raise minimum required version to 1.13
 * Note fastboot compat in README.md
 * Document destinationElement

@chrislopresto chrislopresto merged commit 18182d5 into yapplabs:master Jun 8, 2016

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@chrislopresto

chrislopresto Jun 8, 2016

Contributor

Onward!

Contributor

chrislopresto commented Jun 8, 2016

Onward!

@mixonic mixonic deleted the bantic:fastboot-without-private-apis branch Jun 8, 2016

@bantic bantic referenced this pull request Jun 8, 2016

Closed

fastboot compatibility #47

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