Skip to content
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

[BUGFIX release] Prevent errors in ember-engines + 3.1 + proxies. #16613

Merged
merged 1 commit into from
May 8, 2018

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented May 4, 2018

Setup:

  • In Ember 3.1 any service that has an unknownProperty method will trigger an assertion if properties are accessed directly and the unknownProperty implementation does not return undefined (which is commonly true for things like "features" service which always returns true or false).

  • Ember Engines registers any service instances from the host app into the engines container (with instantiate: false set). This is the mechanism by which engineDependencies works.

  • When a service is looked up (regardless of instantiate: false) the container looks for and calls the _onLookup method so that the class can validate all of its injections.

Combining these two pieces means that any engines with an engineDependencies provided service that has an unknownProperty will throw an error when the service is looked up.

Fixes #16610

Setup:

* In Ember 3.1 any service that has an `unknownProperty` method will
trigger an assertion if properties are accessed directly and the
`unknownProperty` implementation does not return `undefined` (which is
commonly true for things like "features" service which always returns
true or false).

* Ember Engines registers any service instances from the host app into
the engines container (with `instantiate: false` set). This is the
mechanism by which `engineDependencies` works.

* When a service is looked up (regardless of `instantiate: false`) the
container looks for and calls the `_onLookup` method so that the class
can validate all of its injections.

Combining these two pieces means that any engines with an
`engineDependencies` provided service that has an `unknownProperty`
will throw an error when the service is looked up.
@@ -109,6 +109,7 @@ function makeCtor(base) {
property === 'didAddListener' ||
property === 'didRemoveListener' ||
property === 'isDescriptor' ||
property === '_onLookup' ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we were just going to exclude type 'function' in addition to undefined

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did that, but as you can see in this case the unknownProperty always returns non-null non-undefined.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't it return a function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not on the instance which is what this test is doing. The test replicates how ember-engines passes down services from the host app into the engines own container. Roughly akin to:

instance = hostAppOwner.lookup(name);
engineOwner.register(name, instance, { instantiate: false })

container.lookup uses factoryFor first (so it can later call .create()) and that does the _onLookup probe on the result (an instance in this case).

@rwjblue rwjblue merged commit 4c976f8 into emberjs:master May 8, 2018
@rwjblue rwjblue deleted the fix-engines-regression branch May 8, 2018 22:31
@toranb
Copy link
Contributor

toranb commented Jul 7, 2018

@rwjblue It doesn't appear this was ever merged into ember v3.1X (just ember v3.2+)

https://github.com/emberjs/ember.js/blob/coreobject-3-1/packages/ember-runtime/lib/system/core_object.js#L120

If you open the changelog and search for this [BUGFIX] Prevent errors in ember-engines + 3.1 + proxies. you don't see it as part of anything v3.1

@jamescdavis
Copy link
Contributor

☝️

@rwjblue
Copy link
Member Author

rwjblue commented Sep 3, 2018

:( sorry y’all. At this point (3.4 is latest) we probably won’t ship an update to 3.1 for this though...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants