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

create and (trying to) fix async loading of Routes #19456

Open
wants to merge 2 commits into
base: failing-test-case-for-engines
Choose a base branch
from

Conversation

sly7-7
Copy link
Contributor

@sly7-7 sly7-7 commented Mar 10, 2021

This is a follow up of #19266

@rwjblue
Copy link
Member

rwjblue commented Mar 10, 2021

Chatted a bit with @sly7-7 about this in discord this morning, the conclusion we came to was that Ember should never attempt to enter into a loading or error route that itself is not loaded.

I believe that @sly7-7 is going to update this code:

function routeHasBeenDefined(owner: Owner, router: any, localName: string, fullName: string) {
let routerHasRoute = router.hasRoute(fullName);
let ownerHasRoute =
owner.hasRegistration(`template:${localName}`) || owner.hasRegistration(`route:${localName}`);
return routerHasRoute && ownerHasRoute;
}

Check that getRoute returns a non-promise before returning true.

Something like:

function routeHasBeenDefined(owner: Owner, router: any, localName: string, fullName: string) {
  let routerHasRoute = router.hasRoute(fullName);
    
  if (routeHasRoute && !isPromise(router.getRoute(fullName))) {
    let ownerHasRoute =
      owner.hasRegistration(`template:${localName}`) || owner.hasRegistration(`route:${localName}`);

    return ownerHasRoute;
  }

  return false;
}

@sly7-7 sly7-7 force-pushed the failing-test-case-for-engines branch from cf582e6 to 0fba94d Compare March 10, 2021 16:41
assert.ok(true, '/posts/1 has been handled');
let router = this.applicationInstance.lookup('router:main');
run(() => router.send('editPost'));
await runLoopSettled();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rwjblue I have doubt about that artificial synchronisation

@@ -941,6 +946,14 @@ class LoadingTests extends ApplicationTestCase {
assert.equal(childcount, 2);
});
}

['@test What about loading states'](assert) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a reminder to me, to remove this test

import { isDestroying } from '@glimmer/destroyable';
import RSVP from 'rsvp';

export default class LazyRouter extends Router {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if the name fits well.

@sly7-7 sly7-7 changed the title make model loading test compatible with async loading create and (trying to) fix async loading of Routes Mar 10, 2021
@sly7-7
Copy link
Contributor Author

sly7-7 commented Mar 15, 2021

@rwjblue Hi, so I've played with that PR, and I'm blocked, mainly because I miss knowledge on the testing infrastructure, and also when it comes to engines.
It seems to me that I need some waiters helpers (in order to check the loading states), but I couldn't find a way to make them work.
Also, I noted this test: https://github.com/emberjs/ember.js/blob/master/packages/@ember/-internals/glimmer/tests/integration/application/engine-test.js#L26 use quite but not the same routerOptions() as in the 'helper' I created. I was wondering if their purposes are the same or not.
Let me know when we can eventually schedule a chat to talk about that, or if you see what I mean here, you could also help me with tiny snippets here. As you prefer :)

@kategengler
Copy link
Member

Is this still relevant?

@sly7-7
Copy link
Contributor Author

sly7-7 commented May 29, 2024

@kategengler I'm sorry I didn't notice your question until now 😞 I don't know if this is still relevant, as far as I remember, an error could happen when a route is loaded by an engine.
There was also a PR in the router tildeio/router.js#322 and quickly looking at the code this one may be still relevant (but lack of tests that I failed to write).
I don't know if the router in ember has been touched sinced 3 years now, but if nothing has changed, then maybe this PR also could be relevant. I've never encountered this use-case in prod app, so I couldn't tell if the bug mentioned could really occur.

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.

3 participants