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

module unification feature flag #15350

Open
mixonic opened this Issue Jun 11, 2017 · 7 comments

Comments

Projects
None yet
4 participants
@mixonic
Member

mixonic commented Jun 11, 2017

Basic support for module unification already exists in Ember. However there are two parts to the functionality that will require minor changes.

Eventually, we want glimmer-di to replace Ember's container, however blocking module unification on that task seems unwise. Instead, this feature flag is an iterative step that should unblock progress even if the implementation isn't the exact final form we want it to be in.

This feature flag will exist to flag two pieces of functionality:

  • Local lookup. Still under investigation, but there may be minor changes required to properly support local lookup with the Ember resolver API.
  • Module unification namespaces, aka support for MU addons. For example, invoking a component as {{my-addon::some-component}} or adding a service dependency as session: Ember.inject.service('my-addon::session').

Supporting MU namespaces

Given a component invoked like so:

{{my-addon::date-picker}}

What do we call lookup with? Today this codepath is blocked by an assertion, but if that assertion is removed Ember would call:

applicationInstance.lookup('template:components/my-addon::date-picker')

This is pretty much nonsense. It could be parsed, but avoiding string parsing and re-mangling is to be avoided. Additionally, adding any other special sigils or syntax to the lookup strings is not desired especially since lookup is itself public API. Once glimmer-di lands, it would be ideal to simply pass a partial specifier. For example:

applicationInstance.lookup({
  type: 'template',
  collection: 'components',
  rootName: 'my-addon',
  name: 'date-picker'
});

However the goal of this feature flag is to continue using the Ember container as it exists today. To avoid adding any public API and avoid messing with strings, a private API on the container is proposed. Something like:

lookupByRootName(applicationInstance, 'template', 'components', 'my-addon::date-picker');

This API can do whatever internal machinations required to get the namespaced my-addon::date-picker string to be a property lookup in the container, and let us move forward with addon support for module unification without adding public API. The serialized path passed to the resolver can be the absolute specifier serialized:

template:/my-addon/components/date-picker

This requires some knowledge in Ember about the module unification config, but that temporary abstraction leak will be eventually plugged by glimmer-di supporting partial specifiers as lookups.

Lastly, whatever current assertions for using :: in component and service names that exist must be moved out of the main Ember codebase. Existing resolvers (Ember's default resolver and the ember-resolver classic resolver) may need to have assertions added regarding their lack of support for namespaces.

@mixonic

This comment has been minimized.

Member

mixonic commented Jul 3, 2017

Feature flag with local lookup landed in #15368.

@kevinansfield

This comment has been minimized.

Contributor

kevinansfield commented Jul 11, 2017

Using the module unification feature flags we've run into an issue using {{link-to}} linking to routes that have auto-generated index controllers/routes. An example of the generated error:

Assertion Failed: You attempted to define a `{{link-to "settings.apps"}}` but did not pass the parameters required for generating its dynamic segments. Cannot read property 'defaultType' of undefined
Error
    at assert (http://ghost.blog/ghost/assets/vendor-18205fff5f9962dca29684ca13790d9d.js:36932:15)
    at Class.computeLinkToComponentHref (http://ghost.blog/ghost/assets/vendor-18205fff5f9962dca29684ca13790d9d.js:39921:55)
    at ComputedPropertyPrototype.get (http://ghost.blog/ghost/assets/vendor-18205fff5f9962dca29684ca13790d9d.js:48902:28)
    at get (http://ghost.blog/ghost/assets/vendor-18205fff5f9962dca29684ca13790d9d.js:48236:20)
    at RootPropertyReference.compute (http://ghost.blog/ghost/assets/vendor-18205fff5f9962dca29684ca13790d9d.js:44999:34)
    at RootPropertyReference.value (http://ghost.blog/ghost/assets/vendor-18205fff5f9962dca29684ca13790d9d.js:44866:45)
    at ReferenceCache.initialize (http://ghost.blog/ghost/assets/vendor-18205fff5f9962dca29684ca13790d9d.js:23611:52)
    at ReferenceCache.peek (http://ghost.blog/ghost/assets/vendor-18205fff5f9962dca29684ca13790d9d.js:23585:29)
    at DynamicAttribute.flush (http://ghost.blog/ghost/assets/vendor-18205fff5f9962dca29684ca13790d9d.js:25460:37)
    at ComponentElementOperations.flush (http://ghost.blog/ghost/assets/vendor-18205fff5f9962dca29684ca13790d9d.js:25286:44)

Adding empty routes/controller files for auto-generated objects acts as a workaround.

For the routes I traced it back to handlerInfo.handler not being present for the auto-generated settings.apps.index route which throws the above error in _getQPMeta

@mixonic

This comment has been minimized.

Member

mixonic commented Aug 16, 2017

@kevinansfield I believe that was an un-related regression in canary. We should confirm.

@mixonic

This comment has been minimized.

Member

mixonic commented Sep 8, 2017

Next steps for addons: ember-cli/ember-resolver#214

Add tests and support for namespaces. Presume resolve can be called with three arguments (the third is new).

  • lookupString: This must remain the same for things already supported by the resolver API. Ember cannot change what is passed for this value when looking up {{foo-bar}} for example.
  • referrer: This we've already added in the GlimmerWrapper and in Ember behind a feature flag. We use it to pass a referrer for templates.
  • literalString: This is a new argument that must be added here and behind the Ember feature flag. It is "exactly what the user typed". Ember can pass this argument all the time or only when a namespace is specifier (when :: is in the string).
resolve(lookupString, referrer, literalString);

For inject.service('foo-bar');
glimmerWrapperResolver.resolve('service:foo-bar');
gilmmerResolver.resolve('service:foo-bar');

For inject.service('addon-name::service-name');
glimmerWrapperResolver.resolve('service', null, 'addon-name::service-name');
glimmerResolver.resolve('service:/addon-name/services/service-name')

For {{foo-bar}}
glimmerWrapperResolver.resolve('template:components/foo-bar', referrer)
glimmerResolver.resolve('template:foo-bar', referrer)

for {{addon-name::component-name}}
glimmerWrapperResolver.resolve('template:component/', referrer, 'addon-name::component-name')
glimmerResolver.resolve('template:/addon-name/components/component-name', referrer)

In this repo we should add tests and support for calling resolve with the third argument for namespaced services and component. This probably means extending the API of lookup and factoryFor to:

factoryFor(lookupString, {
  referrer,
  literalString
});

lookup(lookupString, {
  referrer,
  literalString
});

mixonic added a commit to ember-cli/ember-resolver that referenced this issue Nov 24, 2017

Add support to glimmer-wrapper for MU namespaces
resolver.resolve now takes a third argument `rawString` which is the
string used at the invocation site of the lookup. For example for:

```
{{ember-power-select::option}}
```

The lookup should be:

```
resolver.resolve('template:component/', null, 'ember-power-select::option')
``

And for a service example:

```
Ember.service.inject('auth-addon::main-service')
```

The lookup would be:

```
resolver.resolve('service', null, 'auth-addon::main-service')
```

Refs: #214
Refs: emberjs/ember.js#15350 (comment)

iezer added a commit to 201-created/dangerously-set-unified-resolver that referenced this issue Dec 14, 2017

Add support to glimmer-wrapper for MU namespaces
resolver.resolve can accept a namespace denoted by a double colon in the first argument, for example: 
 - `service:other-namespace::i18n`
 - `template:components/other-namespace::my-component`

For example for:

```
{{ember-power-select::option}}
```

The lookup should be:

```
resolver.resolve('template:component/ember-power-select::option’)
``

And for a service example:

```
Ember.service.inject('auth-addon::main-service')
```

The lookup would be:

```
resolver.resolve('service:auth-addon::main-service’)
```

Refs: ember-cli#214
Refs: emberjs/ember.js#15350 (comment)

iezer added a commit to 201-created/dangerously-set-unified-resolver that referenced this issue Dec 14, 2017

Add support to glimmer-wrapper for MU namespaces
resolver.resolve can accept a namespace denoted by a double colon in the first argument, for example: 
 - `service:other-namespace::i18n`
 - `template:components/other-namespace::my-component`

For example for:

```
{{ember-power-select::option}}
```

The lookup should be:

```
resolver.resolve('template:component/ember-power-select::option’)
``

And for a service example:

```
Ember.service.inject('auth-addon::main-service')
```

The lookup would be:

```
resolver.resolve('service:auth-addon::main-service’)
```

Refs: ember-cli#214
Refs: emberjs/ember.js#15350 (comment)

mixonic added a commit to 201-created/dangerously-set-unified-resolver that referenced this issue Jan 20, 2018

Add support to glimmer-wrapper for MU namespaces
resolver.resolve now takes a third argument `rawString` which is the
string used at the invocation site of the lookup. For example for:

```
{{ember-power-select::option}}
```

The lookup should be:

```
resolver.resolve('template:component/', null, 'ember-power-select::option')
``

And for a service example:

```
Ember.service.inject('auth-addon::main-service')
```

The lookup would be:

```
resolver.resolve('service', null, 'auth-addon::main-service')
```

Refs: ember-cli#214
Refs: emberjs/ember.js#15350 (comment)
@mixonic

This comment has been minimized.

Member

mixonic commented Mar 13, 2018

We have some changes to the implementation, but the basic implementation is behind a feature flag!

@izelnakri

This comment has been minimized.

izelnakri commented Aug 13, 2018

@kevinansfield @mixonic @rwjblue I just had the same link-to bug on Module unification app:

{{link-to "New Post" "admin.posts.new"}}

Ember v3.3.1, ember-resolver 5.0.1

Assertion Failed: You attempted to define a {{link-to "admin.posts.new"}} but did not pass the parameters required for generating its dynamic segments. Cannot read property 'defaultType' of undefined

@drewhamlett

This comment has been minimized.

drewhamlett commented Aug 24, 2018

Same here with with latest canary.

This works

 this.route('uploads', { path: '/' }, function() {
    this.route('show', { path: 'uploads/:id' })
  })

This doesn't

 this.route('uploads', { path: '/' }, function() {
    this.route('show', { path: 'uploads/:id' }, function () {})
  })

I get Uncaught TypeError: Cannot read property 'defaultType' of undefined on the uploads/1 route and on / I get

Assertion Failed: You attempted to define a `{{link-to "uploads.show"}}` but did not pass the parameters required for generating its dynamic segments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment