-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Conversation
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' || |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
☝️ |
:( sorry y’all. At this point (3.4 is latest) we probably won’t ship an update to 3.1 for this though... |
Setup:
In Ember 3.1 any service that has an
unknownProperty
method will trigger an assertion if properties are accessed directly and theunknownProperty
implementation does not returnundefined
(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 whichengineDependencies
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 anunknownProperty
will throw an error when the service is looked up.Fixes #16610