Disable ilios partner test #7744
Conversation
snewcomer
added
🎯 canary
|
Asset Size Report for 1b28270 Modern Builds ☑️ EmberData has not changed in sizeIf any packages had changed sizes they would be listed here. Changeset Full Asset Analysis (Modern)Modern Builds (No Rollup) ☑️ EmberData has not changed in sizeIf any packages had changed sizes they would be listed here. Changeset Full Asset Analysis (Modern) |
|
Performance Report for 1b28270 Scenario - materialization: ☑️ Performance is stable
Scenario - unload: ☑️ Performance is stable
Scenario - destroy: ☑️ Performance is stable
Scenario - add-children: ☑️ Performance is stable
Scenario - unused-relationships: ☑️ Performance is stable
|
|
cc @jrjohnson |
|
Thanks, I'm looking into it now. Initially appears to just be a controller that still has an |
|
Dug deeper into this today - the core issue seems to be that if I have a route class that doesn't inject the store service it will fail when the default import Route from '@ember/routing/route';
import { inject as service } from '@ember/service';
export default class SomeRouter extends Route {
@service session;
beforeModel(transition) {
this.session.requireAuthentication(transition, 'login');
}
}Is now invalid. But injecting the store like; import Route from '@ember/routing/route';
import { inject as service } from '@ember/service';
export default class SomeRouter extends Route {
@service session;
@service store;
beforeModel(transition) {
this.session.requireAuthentication(transition, 'login');
}
}works. I'd guess this is the result of removing the implicit injection in @snewcomer's #7667. This makes sense, but I don't think this is precisely the intended behavior, but I'm not sure if this is an issue here or should be addressed in Ember itself or in docs or blueprints. |
|
@jrjohnson Can you send me the route this is happening on in your codebase |
|
Thanks!
So explicit injection is expected based on the RFC below. |
NVM: I see when I look at the full route the store is expected to be there. I've been working on a codemod for explicit injections, should help folks get past this with fewer hiccups. |
Yup, I'm all for this! What I didn't expect was the need to inject the store when I wasn't calling
Is that in my code? As far as I could see we're not relying on the store anywhere in the route chain up to application, but maybe I missed it? if that's the case then this is a clear issue in our app and easy enough to fix. Otherwise I may try and see if the store can be looked up from the container whenever a default hook expects it to exist. |
|
@jrjohnson you linked https://github.com/ilios/frontend/blob/master/app/routes/curriculum-inventory-reports.js Which contains async model() {
return this.store.findAll('school');
} |
|
Ah hell. Sorry. Extra s. I was aiming for https://github.com/ilios/frontend/blob/master/app/routes/curriculum-inventory-report.js |
|
So to circle back to the actual failures, this was the errors in our test suite.
https://github.com/emberjs/data/runs/4113308601?check_suite_focus=true |
|
That error was masking the real issue because it triggered our error component. It's fixed now so the real issue, that there is no store on that route, is exposed. Edit: it's fixed now. So rerunning the test will show the real error. |
|
@jrjohnson that route should have store, it's using it you just aren't aware of it. Notice that the route definition has a url-param, ember's (not ember-data's) default model-hook behavior is to call |
|
@runspired I agree! However it is super weird to inject the store into code where there is no explicit I think the solution is probably going to be looking up the store in the default route instead of relying on |
|
@jrjohnson were you getting a deprecation before for this behavior? |
|
@igorT not that I can see, our latest run at https://github.com/ilios/frontend/runs/4148647180 doesn't mention anything that I can find about store or injection. |
|
@jrjohnson Could you take a look at this test run when you get a chance? https://github.com/emberjs/data/runs/4168965070?check_suite_focus=true |
|
Yes. |
|
I've tracked this down to https://github.com/emberjs/ember.js/blob/master/packages/%40ember/-internals/routing/lib/system/route.ts#L1400 where the store is expected to exist on the route or take a default and also confirmed that this happens when there is no model hook and also when there is no backing class at all. It doesn't trigger any deprecations in a new ember app with these feature and fails in this simple case as well. I created a sample Ember CLI 3.28.4 app and added two routes to it. Working with with ED 3.28, but fails with 4.0 beta 4. |
|
@jrjohnson How does this look? |
|
@snewcomer it throws the same error as before. It looks like a good incidental change, but I'm not clear on how it solves this issue. I don't think the core problem is with ED at all, the issue is that implicit models didn't get deprecated, but now will suddenly stop working without the store injection. |
|
@jrjohnson There is one more step I need to take care of. Since we did deprecate implicit injection, I was hoping that the deprecation would surface on your test suite. But if they aren't, then I missed something.
|
|
Ah, ok. No I patched in your router code into the sample app I built and still don't see a deprecation, just the assertion failure. |
Lots of failures from that package. I don't have the time to go fix them in the short term. This should get us back to green.