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

[FEATURE ember-htmlbars-local-lookup] #12673

Merged
merged 1 commit into from Dec 4, 2015

Conversation

Projects
None yet
9 participants
@rwjblue
Member

rwjblue commented Dec 3, 2015

Implements local lookup concepts. This new concept is primarily used from the template layer in this initial implementation, but the source option is usable generally.

There are a couple main concepts being added here:

  • Expanding a type:name relative to a given source type:name into a fully qualified type:name (that includes the source info).
  • Expose the currently rendering template's meta information on the RenderEnv.
  • Adding a second layer of lookups in all places that components or helpers are resolved.

expandLocalLookup

We use the expandLocalLookup function on the resolver (when present) to take both the source and target full names and return a new fully qualified full name. By default the resolver will not have an expandLocalLookup function, but an example implementation exists in the ember-htmlbars/tests/integration/local-lookup-test module here.

meta

As of Ember 1.12, metadata has been appended to every template to allow better customization of things like deprecation messages and template compilation errors.

With the changes in this commit that metadata is exposed on the RenderEnv which is present through the various internal glimmer engine hooks (and provides us the source information needed for the local lookups added here).

local + global lookups

The various places that are responsible for looking helpers and components up have been updated to do two layers of lookups. Once for the current global style lookups, and a second for lookups relative to the invocation template.


I have created a small demo twiddle: here

The builds from this PR can also be located here and available from bower at:

"ember": "rwjblue/ember#local-lookup",
@Gaurav0

This comment has been minimized.

Show comment
Hide comment
@Gaurav0

Gaurav0 Dec 3, 2015

Contributor

Is there an RFC for this feature? Also, how would you access a nested component from outside local context?

Awesome twiddle, btw!

Contributor

Gaurav0 commented Dec 3, 2015

Is there an RFC for this feature? Also, how would you access a nested component from outside local context?

Awesome twiddle, btw!

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Dec 3, 2015

Member

@Gaurav0:

Is there an RFC for this feature?

No there is no corresponding RFC for this.

How would you access a nested component from outside local context?

This isn't really changed here, you could invoke {{local-only/x-local}} in a template (for current Ember.Component's), but there is no way to access nested components (other than this PR if/when it lands) via angle bracket invocation (because we do not intend to support <local-only/x-local /> invocation).

Member

rwjblue commented Dec 3, 2015

@Gaurav0:

Is there an RFC for this feature?

No there is no corresponding RFC for this.

How would you access a nested component from outside local context?

This isn't really changed here, you could invoke {{local-only/x-local}} in a template (for current Ember.Component's), but there is no way to access nested components (other than this PR if/when it lands) via angle bracket invocation (because we do not intend to support <local-only/x-local /> invocation).

@mixonic

View changes

Show outdated Hide outdated packages/container/lib/container.js
@@ -232,12 +245,23 @@ function buildInjections(/* container, ...injections */) {
return hash;
}
function factoryFor(container, fullName) {
function factoryFor(container, _fullName, options) {
var registry = container.registry;

This comment has been minimized.

@mixonic

mixonic Dec 3, 2015

Member

let

@mixonic

View changes

Show outdated Hide outdated packages/container/tests/test-helpers/build-owner.js
@@ -7,7 +7,7 @@ export default function buildOwner(props) {
let Owner = EmberObject.extend(RegistryProxy, ContainerProxy, {
init() {
this._super(...arguments);
let registry = this.__registry__ = new Registry();
let registry = this.__registry__ = new Registry(this._registryOptions);

This comment has been minimized.

@mixonic

mixonic Dec 3, 2015

Member

_registryOptions is only for tests? Can you leave a comment?

@mixonic

mixonic Dec 3, 2015

Member

_registryOptions is only for tests? Can you leave a comment?

This comment has been minimized.

@rwjblue

rwjblue Dec 3, 2015

Member

I do not mind adding a comment, but I am unsure what it would say since this entire file is only for tests (it is a testing helper that is imported via import buildOwner from 'container/tests/test-helpers/build-owner').

@rwjblue

rwjblue Dec 3, 2015

Member

I do not mind adding a comment, but I am unsure what it would say since this entire file is only for tests (it is a testing helper that is imported via import buildOwner from 'container/tests/test-helpers/build-owner').

This comment has been minimized.

@mixonic

mixonic Dec 3, 2015

Member

Blup, correct you are :-)

@mixonic

mixonic Dec 3, 2015

Member

Blup, correct you are :-)

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Dec 3, 2015

Member

This has been updated (along with the demo twiddle, S3 assets, and bower assets) to do the following:

  • Use EmptyObject for caching local lookups (instead of dictionary) since we do not need to delete from it.
  • Ensure that local lookups "win" out over global lookups. I had previously thought this was a backwards incompatible change, but have since realized it is not (thanks @mixonic / @mmun / @dgeb for convincing me). It is not a breaking change because we do not intend to add expandLocalLookup to the existing ember-resolver/resolver, and instead we plan to make a new resolver ember-resolver/pods (or some other better name). Since the new functionality is based on a new resolver base class, it is entirely opt-in (and therefore not breaking).
Member

rwjblue commented Dec 3, 2015

This has been updated (along with the demo twiddle, S3 assets, and bower assets) to do the following:

  • Use EmptyObject for caching local lookups (instead of dictionary) since we do not need to delete from it.
  • Ensure that local lookups "win" out over global lookups. I had previously thought this was a backwards incompatible change, but have since realized it is not (thanks @mixonic / @mmun / @dgeb for convincing me). It is not a breaking change because we do not intend to add expandLocalLookup to the existing ember-resolver/resolver, and instead we plan to make a new resolver ember-resolver/pods (or some other better name). Since the new functionality is based on a new resolver base class, it is entirely opt-in (and therefore not breaking).
@dgeb

View changes

Show outdated Hide outdated packages/container/lib/container.js
let fullName = _fullName;
if (isEnabled('ember-htmlbars-local-lookup')) {
if (options && options.source) {

This comment has been minimized.

@dgeb

dgeb Dec 3, 2015

Member

options should always be initialized via line 185.

@dgeb

dgeb Dec 3, 2015

Member

options should always be initialized via line 185.

@dgeb

View changes

Show outdated Hide outdated packages/container/lib/container.js
let fullName = _fullName;
if (isEnabled('ember-htmlbars-local-lookup')) {
if (options && options.source) {

This comment has been minimized.

@dgeb

dgeb Dec 3, 2015

Member

Consider following the same pattern with options as in lookup().

Edit: I'm not so sure now, given the varying patterns of options usage throughout the module.

@dgeb

dgeb Dec 3, 2015

Member

Consider following the same pattern with options as in lookup().

Edit: I'm not so sure now, given the varying patterns of options usage throughout the module.

@dgeb

View changes

Show outdated Hide outdated packages/container/lib/registry.js
assert('fullName must be a proper full name', this.validateFullName(fullName));
var factory = resolve(this, this.normalize(fullName));
var factory = resolve(this, this.normalize(fullName), options);

This comment has been minimized.

@dgeb

dgeb Dec 3, 2015

Member

let

@dgeb

dgeb Dec 3, 2015

Member

let

@Gaurav0

This comment has been minimized.

Show comment
Hide comment
@Gaurav0

Gaurav0 Dec 3, 2015

Contributor

@rwjblue According to rfcs, "substantial" changes to Ember require an RFC. A substantial change is defined there as:
    

  • Any new feature that creates new API surface area, and would require a feature flag if introduced.
Contributor

Gaurav0 commented Dec 3, 2015

@rwjblue According to rfcs, "substantial" changes to Ember require an RFC. A substantial change is defined there as:
    

  • Any new feature that creates new API surface area, and would require a feature flag if introduced.
@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Dec 3, 2015

Member

@Gaurav0 - Thanks for pointing this out and confirming that this doesn't need an RFC! There is no new public API surface area introduced by this pull request.

Member

rwjblue commented Dec 3, 2015

@Gaurav0 - Thanks for pointing this out and confirming that this doesn't need an RFC! There is no new public API surface area introduced by this pull request.

@dgeb

View changes

Show outdated Hide outdated packages/container/lib/registry.js
var registry = new Registry();
// the twitter factory is added to the module system
registry.expandLocalLookup('component:post-title', 'template:post) // => component:post/post-title

This comment has been minimized.

@dgeb

dgeb Dec 3, 2015

Member

options should be an object in this example:

registry.expandLocalLookup('component:post-title', {source: 'template:post'}); // => component:post/post-title
@dgeb

dgeb Dec 3, 2015

Member

options should be an object in this example:

registry.expandLocalLookup('component:post-title', {source: 'template:post'}); // => component:post/post-title
@mmun

This comment has been minimized.

Show comment
Hide comment
@mmun

mmun Dec 3, 2015

Member

@Gaurav0 This is behind a feature flag so we won't enable it before it has been discussed in an RFC. I think the work here was quite technical so Robert wanted feedback from Dan ASAP. We're confident in this feature so I expect it to go through the RFC process quickly.

Member

mmun commented Dec 3, 2015

@Gaurav0 This is behind a feature flag so we won't enable it before it has been discussed in an RFC. I think the work here was quite technical so Robert wanted feedback from Dan ASAP. We're confident in this feature so I expect it to go through the RFC process quickly.

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Dec 3, 2015

Member

@rwjblue LGTM except for the minor nits mentioned and the need for more unit testing of the changes to the Registry and Container.

Member

dgeb commented Dec 3, 2015

@rwjblue LGTM except for the minor nits mentioned and the need for more unit testing of the changes to the Registry and Container.

@workmanw

This comment has been minimized.

Show comment
Hide comment
@workmanw

workmanw Dec 3, 2015

Contributor

So one concern I had from a functional standpoint was ultimately losing the ability to use the component helper ({{component}}) to include a nested component. With the example that @rwjblue provided, I was/am concerned about having no way to use local-only/x-local outside of the local-only hierarchy.

So I tested with @rwjblue's example:

{{! global-only/template.hbs --}}

{{x-global}}
{{!-- both of these continue work (this is expected) --}}
{{component "local-only/x-local"}}
{{local-only/x-local}}

However, I know that with angle-brackets the later option will not be supported because it contains a slash. My question is will I still be able to do: <x-component "local-only/x-local"></x-component>?

Contributor

workmanw commented Dec 3, 2015

So one concern I had from a functional standpoint was ultimately losing the ability to use the component helper ({{component}}) to include a nested component. With the example that @rwjblue provided, I was/am concerned about having no way to use local-only/x-local outside of the local-only hierarchy.

So I tested with @rwjblue's example:

{{! global-only/template.hbs --}}

{{x-global}}
{{!-- both of these continue work (this is expected) --}}
{{component "local-only/x-local"}}
{{local-only/x-local}}

However, I know that with angle-brackets the later option will not be supported because it contains a slash. My question is will I still be able to do: <x-component "local-only/x-local"></x-component>?

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Dec 3, 2015

Member

My question is will I still be able to do: <x-component "local-only/x-local"></x-component>

I am unsure if there are plans to implement an angle bracket version of the component helper or if it were implemented if it would allow for nested invocation like your example. This PR does not modify the behavior of angle bracket components at all.

Member

rwjblue commented Dec 3, 2015

My question is will I still be able to do: <x-component "local-only/x-local"></x-component>

I am unsure if there are plans to implement an angle bracket version of the component helper or if it were implemented if it would allow for nested invocation like your example. This PR does not modify the behavior of angle bracket components at all.

@workmanw

This comment has been minimized.

Show comment
Hide comment
@workmanw

workmanw Dec 3, 2015

Contributor

This PR does not modify the behavior of angle bracket components at all.

Sorry, it's not always clear to me where I should give this feedback. Someone might argue this question/feedback is about how components are looked up, and others might say it's more specific to angle-bracket use case.

Anyways, this PR looks great, I'm excited to use it. It'll clean up our routable-component "shim templates" a lot. Thank you!

Contributor

workmanw commented Dec 3, 2015

This PR does not modify the behavior of angle bracket components at all.

Sorry, it's not always clear to me where I should give this feedback. Someone might argue this question/feedback is about how components are looked up, and others might say it's more specific to angle-bracket use case.

Anyways, this PR looks great, I'm excited to use it. It'll clean up our routable-component "shim templates" a lot. Thank you!

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Dec 3, 2015

Member

Sorry, it's not always clear to me where I should give this feedback.

No worries! I'm just not sure of the answer to your question so all I am certain of is that the behavior isn't changing from what is on master now...

Member

rwjblue commented Dec 3, 2015

Sorry, it's not always clear to me where I should give this feedback.

No worries! I'm just not sure of the answer to your question so all I am certain of is that the behavior isn't changing from what is on master now...

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Dec 4, 2015

Member

Just pushed an update that fixes the various inline comments here, and adds test coverage for the container/resolver parts.

Member

rwjblue commented Dec 4, 2015

Just pushed an update that fixes the various inline comments here, and adds test coverage for the container/resolver parts.

});
assert.equal(result, 'foo:qux/bar');
});

This comment has been minimized.

@dgeb

dgeb Dec 4, 2015

Member

We need to verify the prioritization given to the resolver, then to the fallback registry, like in https://github.com/rwjblue/ember.js/blob/local-lookup/packages/container/tests/registry_test.js#L316-L340

Something like this (I haven't actually tried to run this):

QUnit.test('`expandLocalLookup` is handled by the resolver, then by the fallback registry, if available', function(assert) {
  assert.expect(9);

  let fallback = {
    expandLocalLookup(targetFullName, sourceFullName) {
      assert.ok(true, 'expandLocalLookup is called on the resolver');
      assert.equal(targetFullName, 'foo:bar', 'the targetFullName was passed through');
      assert.equal(sourceFullName, 'baz:qux', 'the sourceFullName was passed through');

      return 'foo:qux/bar-fallback';
    }
  };

  let resolver = {
    expandLocalLookup(targetFullName, sourceFullName) {
      assert.ok(true, 'expandLocalLookup is called on the resolver');
      assert.equal(targetFullName, 'foo:bar', 'the targetFullName was passed through');
      assert.equal(sourceFullName, 'baz:qux', 'the sourceFullName was passed through');

      return 'foo:qux/bar-resolver';
    }
  };

  let registry = new Registry({
    fallback,
    resolver
  });

  let result = registry.expandLocalLookup('foo:bar', {
    source: 'baz:qux'
  });

  assert.equal(result, 'foo:qux/bar-resolver', 'handled by the resolver');

  registry.resolver = null;

  result = registry.expandLocalLookup('foo:bar', {
    source: 'baz:qux'
  });

  assert.equal(result, 'foo:qux/bar-fallback', 'handled by the fallback registry');

  registry.fallback = null;

  result = registry.expandLocalLookup('foo:bar', {
    source: 'baz:qux'
  });

  assert.equal(result, null, 'null is returned by default when no resolver or fallback registry is present');
});
@dgeb

dgeb Dec 4, 2015

Member

We need to verify the prioritization given to the resolver, then to the fallback registry, like in https://github.com/rwjblue/ember.js/blob/local-lookup/packages/container/tests/registry_test.js#L316-L340

Something like this (I haven't actually tried to run this):

QUnit.test('`expandLocalLookup` is handled by the resolver, then by the fallback registry, if available', function(assert) {
  assert.expect(9);

  let fallback = {
    expandLocalLookup(targetFullName, sourceFullName) {
      assert.ok(true, 'expandLocalLookup is called on the resolver');
      assert.equal(targetFullName, 'foo:bar', 'the targetFullName was passed through');
      assert.equal(sourceFullName, 'baz:qux', 'the sourceFullName was passed through');

      return 'foo:qux/bar-fallback';
    }
  };

  let resolver = {
    expandLocalLookup(targetFullName, sourceFullName) {
      assert.ok(true, 'expandLocalLookup is called on the resolver');
      assert.equal(targetFullName, 'foo:bar', 'the targetFullName was passed through');
      assert.equal(sourceFullName, 'baz:qux', 'the sourceFullName was passed through');

      return 'foo:qux/bar-resolver';
    }
  };

  let registry = new Registry({
    fallback,
    resolver
  });

  let result = registry.expandLocalLookup('foo:bar', {
    source: 'baz:qux'
  });

  assert.equal(result, 'foo:qux/bar-resolver', 'handled by the resolver');

  registry.resolver = null;

  result = registry.expandLocalLookup('foo:bar', {
    source: 'baz:qux'
  });

  assert.equal(result, 'foo:qux/bar-fallback', 'handled by the fallback registry');

  registry.fallback = null;

  result = registry.expandLocalLookup('foo:bar', {
    source: 'baz:qux'
  });

  assert.equal(result, null, 'null is returned by default when no resolver or fallback registry is present');
});

This comment has been minimized.

@rwjblue

rwjblue Dec 4, 2015

Member

@dgeb - thank you! Updated.

@rwjblue

rwjblue Dec 4, 2015

Member

@dgeb - thank you! Updated.

[FEATURE ember-htmlbars-local-lookup]
Implements local lookup concepts. This new concept is primarily
used from the template layer in this initial implementation,
but the `source` option is usable generally.

There are a couple main concepts being added here:

* Expanding a `type:name` relative to a given source `type:name` into a
  fully qualified `type:name` (that includes the source info).
* Expose the currently rendering `template`'s `meta` information on the
  `RenderEnv`.
* Adding a second layer of lookups in all places that components or
  helpers are resolved.

\#### `expandLocalLookup`

We use the `expandLocalLookup` function on the resolver (when present)
to take both the source and target full names and return a new fully
qualified full name. By default the resolver will not have an
`expandLocalLookup` function, but an example implementation exists in
the `ember-htmlbars/tests/integration/local-lookup-test` module here.

\#### `meta`

As of Ember 1.12, metadata has been appended to every template to allow
better customization of things like deprecation messages and template
compilation errors.

With the changes in this commit that metadata is exposed on the
`RenderEnv` which is present through the various internal glimmer engine
hooks (and provides us the `source` information needed for the local
lookups added here).

\#### local + global lookups

The various places that are responsible for looking helpers and
components up have been updated to do two layers of lookups.  Once for
the current global style lookups, and a second for lookups relative to
the invocation template.
@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Dec 4, 2015

Member

Thanks for adding that test, @rwjblue.

LGTM 👍

Member

dgeb commented Dec 4, 2015

Thanks for adding that test, @rwjblue.

LGTM 👍

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Dec 4, 2015

Member

I did some cursory benchmarking with ember-performance in search of regressions, nothing alarming came up.

Member

mixonic commented Dec 4, 2015

I did some cursory benchmarking with ember-performance in search of regressions, nothing alarming came up.

mixonic added a commit that referenced this pull request Dec 4, 2015

Merge pull request #12673 from rwjblue/local-lookup
[FEATURE ember-htmlbars-local-lookup]

@mixonic mixonic merged commit f0dafd4 into emberjs:master Dec 4, 2015

1 check passed

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

@mixonic mixonic deleted the rwjblue:local-lookup branch Dec 4, 2015

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Dec 4, 2015

Member

Twiddle updated to use canary (now that this is merged).

Member

rwjblue commented Dec 4, 2015

Twiddle updated to use canary (now that this is merged).

@elwayman02

This comment has been minimized.

Show comment
Hide comment
@elwayman02

elwayman02 Dec 18, 2015

Contributor

@rwjblue shouldn't this be wrapped in isEnabled?

@rwjblue shouldn't this be wrapped in isEnabled?

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Dec 18, 2015

Member

Hmm, no? This doesn't add new API, it just grabs the source moduleName if present. Are you seeing an error or something? If you are, can you report with a demo or something so I can help track it down?

Member

rwjblue replied Dec 18, 2015

Hmm, no? This doesn't add new API, it just grabs the source moduleName if present. Are you seeing an error or something? If you are, can you report with a demo or something so I can help track it down?

This comment has been minimized.

Show comment
Hide comment
@elwayman02

elwayman02 Dec 18, 2015

Contributor

Not seeing an error, just noticed that you wrapped this exact same block of code in isEnabled in the component.js file above, so I was wondering if this one had been missed.

Contributor

elwayman02 replied Dec 18, 2015

Not seeing an error, just noticed that you wrapped this exact same block of code in isEnabled in the component.js file above, so I was wondering if this one had been missed.

@rmmmp

This comment has been minimized.

Show comment
Hide comment
@rmmmp

rmmmp Jan 13, 2016

@rwjblue Not sure if mentioned elsewhere but will deeply nested components look like this?

app/routes/posts/components/post-header/component.js
app/routes/posts/components/post-header/template.hbs
app/routes/posts/components/post-header/components/post-header-tabs/component.js
app/routes/posts/components/post-header/components/post-header-tabs/template.hbs

or is it like this?

app/routes/posts/components/post-header/component.js
app/routes/posts/components/post-header/template.hbs
app/routes/posts/components/post-header/post-header-tabs/component.js
app/routes/posts/components/post-header/post-header-tabs/template.hbs

rmmmp commented Jan 13, 2016

@rwjblue Not sure if mentioned elsewhere but will deeply nested components look like this?

app/routes/posts/components/post-header/component.js
app/routes/posts/components/post-header/template.hbs
app/routes/posts/components/post-header/components/post-header-tabs/component.js
app/routes/posts/components/post-header/components/post-header-tabs/template.hbs

or is it like this?

app/routes/posts/components/post-header/component.js
app/routes/posts/components/post-header/template.hbs
app/routes/posts/components/post-header/post-header-tabs/component.js
app/routes/posts/components/post-header/post-header-tabs/template.hbs
@tomdale

This comment has been minimized.

Show comment
Hide comment
@tomdale

tomdale Jan 16, 2016

Member

@rmmpadres The top one is what is planned, but is orthogonal to this PR. This PR just implements the local lookup and it is up to the resolver to determine how that local lookup is resolved.

Member

tomdale commented Jan 16, 2016

@rmmpadres The top one is what is planned, but is orthogonal to this PR. This PR just implements the local lookup and it is up to the resolver to determine how that local lookup is resolved.

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