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-module-unification] Initial implementation #15368

Merged
merged 1 commit into from Jul 1, 2017

Conversation

Projects
None yet
3 participants
@iezer
Contributor

iezer commented Jun 16, 2017

To support module unification, we must modify the container and registry to pass the source as second parameter to the resolver. This PR introduces a new feature flag called ember-module-unification which does just that.

I'll follow up with some PRs in the ember-resolver and glimmer-resolver repos, and an end-to-end example of an Ember app using module unification and private components.

When the feature flag is on, this change breaks two tests related to expandLocalLookup. I'm looking at a way to support the new functionality and make those tests pass. While I'm doing that, I'd appreciate some feedback on the overall design here.

EDIT

Related PRs:

@mixonic @rwjblue

@mixonic

Left comments for a few minor items. The testing and feature flag stuff can be improved but the functionality here looks good at a glance. Please do cross-link to the PRs and issues elsewhere.

@@ -1,5 +1,5 @@
/* globals Proxy */
import { assert } from 'ember-debug';
import { assert, isFeatureEnabled } from 'ember-debug';

This comment has been minimized.

@mixonic

mixonic Jun 21, 2017

Member

@rwjblue am I correct in suggesting this should be using ember/features now? Like so:

import { EMBER_MODULE_UNIFICATION } from 'ember/features';

source

Hm I don't think we have any docs on this in CONTRIBUTING.md.

// with ember-module-unification, if expandLocalLookup returns something,
// pass it to the resolve without the source
fullName = expandedFullName;
delete options.source;

This comment has been minimized.

@mixonic

mixonic Jun 21, 2017

Member

I wonder if here and at the other delete call further down we should not be mutating the passed-in object? Instead allocate a new one.

return name;
}
return options && options.source ? `${options.source}:${name}` : name;

This comment has been minimized.

@mixonic

mixonic Jun 21, 2017

Member

Can you wrap the test here in parens just for legibility?

return (options && options.source) ? `${options.source}:${name}` : name;
let container = registry.container();
strictEqual(container.factoryFor('component:my-input', { source: src }).class, PrivateComponent, 'The correct factory was provided');

This comment has been minimized.

@mixonic

mixonic Jun 21, 2017

Member

Tests are passed assert as an argument and should use that over the global assertion methods. For example:

QUnit.test('The container can pass a source to factoryFor', function(assert) {
  /* ... */
  assert.strictEqual(
    container.factoryFor('component:my-input', {source: src}).class, PrivateComponent,
    'The correct factory was provided'
  );
  /* ... */

In fact re-using the string 'component:my-input' instead of re-typing it would clean things up as well, as would using source instead of src as the source string variable name (then this can be {source}).

let container = registry.container();
strictEqual(container.factoryFor('component:my-input', { source: src }).class, PrivateComponent, 'The correct factory was provided');
strictEqual(container.factoryManagerCache[`template:routes/application:component:my-input`].class,

This comment has been minimized.

@mixonic

mixonic Jun 21, 2017

Member

Instead of asserting the contents of the private cache, can the cache be make _ private and here you can assert resolve or the method on the resolver isn't called a second time? or that it is called with a different source? Test the behavior, not the state of the cache?

// pass it to the resolve without the source
normalizedName = expandedFullName;
options = copy(options, true);
delete options.source;

This comment has been minimized.

@mixonic

mixonic Jun 25, 2017

Member

Copying all properties then deleting one is not ideal. It likely causes the object to go into hash table mode which is a slower data structure.

It would be ideal to create an object with only the data we want to pass forward. Kind of silly that we are doing all this work since I think there are no other properties on options :-p

But please just make an object. Even options = {} would be better if there are no other options to copy.

This comment has been minimized.

@iezer

iezer Jun 30, 2017

Contributor

This is scenario that will never get hit in practice because it would only be executed if expandedLocalLookup is implemented and returns something. Are you suggesting that I do options = {};? I'm concerned that some upstream caller might add an addition property to options and expect it to be passed to the resolver.

This comment has been minimized.

@mixonic

mixonic Jun 30, 2017

Member

@iezer Right, options as an argument to lookup is only a contract with the lookup method. We don't document the argument as resolverOptions where it is implied the object is passed transparently.

A patch that build the object up by iterating Object.keys but skipping source would be ok, but really IMO just building the new object (in this case passing an empty object) is fine. Building a whole new object by iterating would be slow, calling the delete after copying is slow, and this is fairly performance sensitive code. Doing the fast and API-conservative thing is ok.

This comment has been minimized.

@iezer

iezer Jun 30, 2017

Contributor

Ok I'll make that change

@mixonic

mixonic requested changes Jun 26, 2017 edited

Talked with @dgeb, and I think this psuedo code would be correct for us to ensure we get the higher precedence for local/private lookups over global when resolving a pair of things:

`
a:/my-app/widgets/x-mark
b:/my-app/widgets/top-level/x-mark
`

let source = 'b:/my-app/widgets/top-level';
let a = di.lookup('a:x-mark', {source})

let pairedB;
if (a) {
  pairedB = di.lookup('b', {source: a})
}

let b;
if (pairedB) {
  b = pairedB;
} else {
  b = di.lookup('b:x-mark', {source})
  if (b) {
    let pairedA = di.lookup('a', {source: b})
    a = pairedA;
  }
}

let paired = [a,b]
return paired;

@iezer iezer force-pushed the iezer:isaac/referrer-to-resolver branch from 1f40d58 to bd5ad81 Jun 30, 2017

@iezer

This comment has been minimized.

Contributor

iezer commented Jun 30, 2017

@mixonic I'm not sure I follow that example. When you call di.lookup you still have no way of knowing if the lookup found the factory with the source or without.

@iezer iezer force-pushed the iezer:isaac/referrer-to-resolver branch 4 times, most recently from e7cdb3f to 14b5e36 Jul 1, 2017

[FEATURE ember-module-unification] Initial work
* Pass `source` to the resolver's `resolve` method as a second argument
* Update the logic for component pairs to ensure local/private
  resolution of a component/template are only matched with another local/private
  resolution.

@mixonic mixonic force-pushed the iezer:isaac/referrer-to-resolver branch from 14b5e36 to 5e0a1fd Jul 1, 2017

@mixonic

mixonic approved these changes Jul 1, 2017

@mixonic

This comment has been minimized.

Member

mixonic commented Jul 1, 2017

I've added the commit prefix and an entry to FEATURES.md along with some other comments in the code. I think this is gtg as a feature flag.

@dgeb

dgeb approved these changes Jul 1, 2017

Thanks for all your hard work @iezer. I think you've walled everything off appropriately behind the feature flag, which should allow us to iterate on the implementation.

@mixonic mixonic changed the title from Module Unification: Pass source to resolver to [FEATURE ember-module-unification] Initial implementation Jul 1, 2017

@mixonic mixonic merged commit d807315 into emberjs:master Jul 1, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment