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

setupLink() is broken for engines #714

Closed
gossi opened this issue Apr 28, 2022 · 2 comments · Fixed by emberjs/ember-test-helpers#1216
Closed

setupLink() is broken for engines #714

gossi opened this issue Apr 28, 2022 · 2 comments · Fixed by emberjs/ember-test-helpers#1216

Comments

@gossi
Copy link
Collaborator

gossi commented Apr 28, 2022

As we figured, we weren't able to use setupLink() from within engines. @buschtoens did a run-down of this (credits to him, I'm just writing it down):

We are hit by this: emberjs/ember.js#11173 (comment)

assert(
'ember-link.setupLink: You have already called `setupLink` once',
!this.owner.hasRegistration('service:link-manager') ||
!(
this.owner.lookup('service:link-manager') instanceof
TestInstrumentedLinkManagerService
)
);

places the normal LinkManagerService in the container.cache and container.factoryManagerCache.
Then the following owner.unregister(…) apparently only removes the registration, but not the instance in the container.cache.

There fix was added to ApplicationInstance, but the owner in our test is not a fully-fledged ApplicationInstance: https://github.com/emberjs/ember.js/pull/12680/files

This seems to be an upstream bug in @ember/test-helpers, here is the trace:

We don’t have an application in render tests, but a resolver, so we hit L57 in
https://github.com/emberjs/ember-test-helpers/blob/a4f710420e8dfba1a4245981483d7cbb5dcc865e/addon-test-support/%40ember/test-helpers/build-owner.ts#L41-L60

Which then builds a registry and owner on its own terms.
https://github.com/emberjs/ember-test-helpers/blob/a4f710420e8dfba1a4245981483d7cbb5dcc865e/addon-test-support/@ember/test-helpers/-internal/build-registry.ts

This of course lacks the unregister patch:
https://github.com/emberjs/ember.js/blob/0da7fedd3767668e7fb01b50757f1a636782fb54/packages/%40ember/engine/instance.ts#L162-L167

@buschtoens
Copy link
Owner

Thanks for copying this over and also thanks for prepping a PR. 😊

Since I consider this a bug in @ember/test-helpers, I've sent in this PR to fix it upstream: emberjs/ember-test-helpers#1216

Hopefully it will be accepted, then we can skip #715. Still thank you for working on this. 🙏

@buschtoens
Copy link
Owner

My PR was merged and released as 2.8.0.

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 a pull request may close this issue.

2 participants