Introduce namespaced component and service invocation #16158

mixonic · 2018-01-21T04:24:47Z

This commit adds support for invoking a component or injecting a service from a module unification namespace. It does so in a very targeted manner, and specifically avoids adding support for namespaces to lower-level APIs like lookup or factoryFor.

Ember itself doesn't perform any resolution steps for a lookup. Instead, it defers resolution to a resolver class provided to the application at runtime. For example this invocation:

{{! app/templates/index.hbs }}
{{app-component}}

Causes a resolver invocation of:

resolve(
  'template:components/app-component', // a "fullName" formatted type:name.
  'app/templates/index' // a referrer for this lookup
);

The resolver API has already been changed behind the module unification feature flag to pass the second argument of "referrer" when appropriate.

A component from another namespace is invoked by putting the namespace before a :: delemiter. For example an invocation:

{{! src/ui/components/app-component/template.hbs }}
{{my-addon::addon-component}}

In order to resolve this namespace the resolver must be passed my-addon is some form or another.

The current Ember container APIs are coupled heavily to the idea of a "fullName". This is a string formatted as type:name, and expected to resolve something. I'm hesitant to expand that syntax to something like type:optional-namespace::name, especially since we would like to replace the strings there with "specifiers" from the glimmer-di project as we adopt more of that project.

Additionally we want existing implementions of the resolver API to not be passed the namespace in the first argument. If they were, they may resolve certain lookups in ways that don't mesh our goals for the :: syntax. For example a custom resolver in someone's app may be splitting on : an expecting a syntax in the string that matches type:name. It is ok to break those resolvers for namespaced lookups, and we would rather they fail in some way than resolve something that is wrong.

Instead, we've made an additive change to the resolver API. The "rawString", the string invoking the lookup, is passed in the third position:

{{! src/ui/components/app-component/template.hbs }}
{{my-addon::addon-component}}

Resolves with:

resolve(
  'template:components/',
  'src/ui/components/app-component/template',
  'my-addon::addon-component'
);
resolve(
  'component',
  'src/ui/components/app-component/template',
  'my-addon::addon-component'
);

Similarly service injections pass their "rawString":

Ember.inject.service('my-addon::addon-service');

Resolves with:

resolve(
  'service',
  undefined,
  'my-addon::addon-service'
);

The resolver is expected to parse the invocation string.

TODO:

Complete a smoke test against ember-resolver master with the MU feature flag.
Add a test for namespaced main lookups of services. store: Ember.inject.service('ember-data') and the addons src/services/main.js would be resolved.
Rationalize what moduleName is on a template and what it should be.

mixonic · 2018-01-21T04:25:59Z

packages/container/lib/container.js

@@ -278,7 +281,7 @@ function isFactoryInstance(container, fullName, { instantiate, singleton }) {
 }

 function instantiateFactory(container, fullName, options) {
-  let factoryManager = EMBER_MODULE_UNIFICATION && options && options.source ? container.factoryFor(fullName, options) : container.factoryFor(fullName);
+  let factoryManager = EMBER_MODULE_UNIFICATION && options ? container.factoryFor(fullName, options) : container.factoryFor(fullName);


I think we can just pass container.factoryFor(fullName, options) all the time.

Turns out some tests related to local lookup fail if we remove this.

mixonic · 2018-01-21T04:26:54Z

packages/container/lib/container.js

@@ -440,3 +446,42 @@ class FactoryManager {
    return instance;
  }
 }
+
+export function parseInjectionString(injectionString) {


document this

mixonic · 2018-01-21T04:26:59Z

packages/container/lib/container.js

+  }
+}
+
+export function lookupWithRawString(container, type, rawString) {


document this

mixonic · 2018-01-21T04:27:05Z

packages/container/lib/container.js

+  }
+}
+
+export function factoryForWithRawString(container, type, rawString) {


document this

mixonic · 2018-01-21T04:27:28Z

packages/container/lib/index.js

@@ -9,4 +9,7 @@ export { default as Registry, privatize } from './registry';
 export {
  default as Container,
  FACTORY_FOR,
+  factoryForWithRawString,
+  lookupWithRawString,
+  RAW_STRING_OPTION_KEY as _RAW_STRING_OPTION_KEY


Confirm these don't become public API

mixonic · 2018-01-21T04:27:57Z

packages/container/lib/registry.js

 import { DEBUG } from 'ember-env-flags';
 import { ENV } from 'ember-environment';

 const VALID_FULL_NAME_REGEXP = /^[^:]+:[^:]+$/;
-const missingResolverFunctionsDeprecation = 'Passing a `resolver` function into a Registry is deprecated. Please pass in a Resolver object with a `resolve` method.';
+
+let missingResolverFunctionsDeprecation = 'Passing a `resolver` function into a Registry is deprecated. Please pass in a Resolver object with a `resolve` method.';


This line should remain whatever it was in master.

mixonic · 2018-01-21T04:29:58Z

packages/container/lib/registry.js

+    if(EMBER_MODULE_UNIFICATION) {
+      /* With a rawString, the fullName doesn't need to contain a : */
+      if (options && options[RAW_STRING_OPTION_KEY] && fullName) {
+        return true;


This needs review with @rwjblue @dgeb I'm not quite sure what to do here. The gist is that previously we required type:name but this check happens at low levels and now type alone is permitted (the name ends up in the rawString). Perhaps we should move this check out of the deep parts of the container instead of removing it in this PR.

@mixonic I believe type alone is no longer permitted so we can remove this.

iezer · 2018-01-30T20:06:36Z

@mixonic FYI I tested this with the latest ember-resolver and the mu-golden-spike app and it works for both components and services. Here's a screenshot:

mixonic · 2018-01-31T16:27:04Z

packages/container/tests/container_test.js

-        specifier: type,
-        rawString: rawString
+        specifier,
+        targetNamespace


This could just be namespace, I don't think target is useful to discern it from anything else?

iezer · 2018-02-01T19:48:55Z

@mixonic and @dgeb We should clarify how we expect the "main" service of an addon to work. In my end-to-end test, If I have an addon called ember-data and I try to inject a service called ember-data, the glimmer resolver expects this to be in /ember-data/src/services/service.js, not /ember-data/src/services/main.js.

If I try to inject a service called ember-data::secondary, The resolver will find it in /ember-data/src/services/secondary.js.

This commit adds support for invoking a component or injecting a service from a module unification namespace. It does so in a very targetted manner, and specifically *avoids* adding support for namespaces to lower-level APIs like lookup or factoryFor. Ember itself doesn't perform any resolution steps for a lookup. Instead, it defers resolution to a resolver class provided to the application at runtime. For example this invocation: ```hbs {{! app/templates/index.hbs }} {{app-component}} ``` Causes a resolver invocation of: ``js resolve( 'template:components/app-component', // a "fullName" formatted type:name. 'app/templates/index' // a referrer for this lookup ); ``` The resolver API has already been changed behind the module unification feature flag to pass the second argument of "referrer" when appropriate. A component from another namespace is invoked by putting the namespace before a :: delemiter. For example an invocation: ```hbs {{! src/ui/components/app-component/template.hbs }} {{my-addon::addon-component}} ``` In order to resolve this namespace the resolver must be passed `my-addon` is some form or another. The current Ember container APIs are coupled heavily to the idea of a "fullName". This is a string formatted as `type:name`, and expected to resolve something. I'm hesitant to expand that syntax to something like `type:optional-namespace::name`, especially since we would like to replace the strings there with "specifiers" from the glimmer-di project as we adopt more of that project. Additionally we want existing implementions of the resolver API to *not* be passed the namespace in the first argument. If they were, they may resolve certain lookups in ways that don't mesh our goals for the `::` syntax. For example a custom resolver in someone's app may be splitting on `:` an expecting a syntax in the string that matches `type:name`. It is ok to break those resolvers for namespaced lookups, and we would rather they fail in some way than resolve something that is wrong. Instead, we've made an *additive* change to the resolver API. The "rawString", the string invoking the lookup, is passed in the third position: ```hbs {{! src/ui/components/app-component/template.hbs }} {{my-addon::addon-component}} ``` Resolves with: ```js resolve( 'template:components/', 'src/ui/components/app-component/template', 'my-addon::addon-component' ); resolve( 'component', 'src/ui/components/app-component/template', 'my-addon::addon-component' ); ``` Similarly service injections pass their "rawString": ```hbs Ember.inject.service('my-addon::addon-service'); ``` Resolves with: ```js resolve( 'service', undefined, 'my-addon::addon-service' ); ``` The resolver is expected to parse the invocation string.

- Still TODO cleanup rename rawString to targetNamespace

mixonic · 2018-03-04T19:35:04Z

Closing this in favor of #16319

mixonic commented Jan 21, 2018

View reviewed changes

mixonic commented Jan 31, 2018

View reviewed changes

iezer force-pushed the namespaced-component-and-service-invocation branch from 6a6c7b7 to 0aed89a Compare January 31, 2018 20:12

iezer mentioned this pull request Jan 31, 2018

Tests for service injection #16203

Merged

iezer force-pushed the namespaced-component-and-service-invocation branch 2 times, most recently from 4dc4587 to e6b3cbb Compare February 1, 2018 19:40

iezer force-pushed the namespaced-component-and-service-invocation branch from e6b3cbb to 6f4ea49 Compare February 5, 2018 22:30

iezer added 9 commits February 12, 2018 20:03

container splits rawString and pass namespace

2c7672f

- Still TODO cleanup rename rawString to targetNamespace

WIP PR cleanup to rename rawString

bbb71ed

Wip remove RAW_STRING_OPTION

510c476

update tests to use rawString

c1201ad

support module unification helpers

5f0f267

Add test for strong namespace requirement

2f24dfe

simplify isValidFullName

1efffe2

WIP support service injection with namespace

5c460a9

mixonic force-pushed the namespaced-component-and-service-invocation branch from 6f4ea49 to 5c460a9 Compare February 13, 2018 04:03

Just cleanup

a110501

mixonic mentioned this pull request Feb 13, 2018

Add referrer capability to the test resolver #16239

Merged

This was referenced Mar 4, 2018

[FEATURE ember-module-unification] Support name-spaced lookups #15967

Closed

Namespaced invocation #16319

Merged

mixonic closed this Mar 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce namespaced component and service invocation #16158

Introduce namespaced component and service invocation #16158

mixonic commented Jan 21, 2018 •

edited

Loading

mixonic Jan 21, 2018

iezer Feb 1, 2018

mixonic Jan 21, 2018

mixonic Jan 21, 2018

mixonic Jan 21, 2018

mixonic Jan 21, 2018

mixonic Jan 21, 2018

mixonic Jan 21, 2018

iezer Jan 31, 2018

iezer commented Jan 30, 2018

mixonic Jan 31, 2018

iezer commented Feb 1, 2018

mixonic commented Mar 4, 2018

Introduce namespaced component and service invocation #16158

Introduce namespaced component and service invocation #16158

Conversation

mixonic commented Jan 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iezer commented Jan 30, 2018

Choose a reason for hiding this comment

iezer commented Feb 1, 2018

mixonic commented Mar 4, 2018

mixonic commented Jan 21, 2018 •

edited

Loading