Bad timing routing issue, failing test

[wagenet]

Not really sure what to name this, or if this test is in the right area,
but it is an issue!

[sly7-7]

If I understand well, this is specifically what the test is about ?

[wagenet]

Correct.

[sly7-7]

@wagenet I've updated (locally) this test to highlight the real issue (it needs an expectDeprecation because of the call to this.replaceWith()).
I don't understand what's going on. The only thing I can tell for now, is that adding this Promise.resolve() (so some kind of asynchrony I guess) causes the router to be in a very bad state, ultimately hitting https://github.com/emberjs/ember.js/blob/master/packages/@ember/-internals/routing/lib/system/router.ts#L1750 with an undefined defaultParentState.

[sly7-7]

Needs something like this:

expectDeprecation(() => {
  this.replaceWith('index');
}, 'Calling replaceWith on a route is deprecated. Use the RouterService instead.');

[wagenet]

Updated.

[sly7-7]

@rwjblue @wagenet, Could you guys please explain me the difference in term of behavior using return Promise.resolve().then(replaceWith()) and await Promise.resolve(); replaceWith()?

Because for me, they are be the same.

The one from Peter's test (somehow makes the test fail)

this.add(
        'route:dummy',
        Route.extend({
          redirect() {
            return RSVP.resolve().then(() => {
              expectDeprecation(() => {
                this.replaceWith('index');
              }, 'Calling replaceWith on a route is deprecated. Use the RouterService instead.');
            });
          },
        })
      );

The one I just tried (the test is green this way).

this.add(
        'route:dummy',
        Route.extend({
          async redirect() {
            await RSVP.resolve();
            expectDeprecation(() => {
              this.replaceWith('index');
            }, 'Calling replaceWith on a route is deprecated. Use the RouterService instead.');
          },
        })
      );

I was thinking maybe await Promise.resolve() would be somehow synchronous, so I modified resolving the Promise after 500ms, and it's working too:

[sly7-7]

I've made some other tiny experiment around return Promise.resolve().then() / await Promise.resolve().
Maybe this could you give you some clue.

 redirect() {
   return new RSVP.Promise((resolve) => {
     count++;
     // later(resolve, 1); // pass
     // later(resolve, 0); // pass
     // next(resolve); // pass
     // run(resolve); // fails
     // resolve(); // fails
   }).then(() => {
     assert.equal(count, 1);
     expectDeprecation(() => {
       this.replaceWith('index');
     }, 'Calling replaceWith on a route is deprecated. Use the RouterService instead.');
   });
 },

 async redirect() {
   await new RSVP.Promise((resolve) => {
     count++;
     // later(resolve, 1); // pass
     // later(resolve, 0); // pass
     // next(resolve); // pass
     // run(resolve); // pass
     // resolve(); // pass
   });
   assert.equal(count, 1);
   expectDeprecation(() => {
     this.replaceWith('index');
   }, 'Calling replaceWith on a route is deprecated. Use the RouterService instead.');
 },

[wagenet]

Possibly it has to do with the difference in the code generated by the transpiler.

[sly7-7]

@wagenet I've had a discussion with @rwjblue on this thing. From the discord chat:
I'm putting that here, so we don't forget:

• at a high level, we have a somewhat fundamental problem when you return a promise that then redirects

seems related to: emberjs/router.js#310 and emberjs/ember-test-helpers#332

• The high level tldr; is that returning a promise from one of these hooks can't prevent the router from continuing to do its work of resolving the various model hooks and what not
  See https://github.com/tildeio/router.js/blob/4f5d411e9ba44efea003bc3159484b4060ebfc2a/lib/router/route-info.ts#L346-L348 (for example)

[locks]

@wagenet is this still relevant?

[wagenet]

@locks I just rebased. Looks like CI still fails.

[kategengler]

Is this still relevant?

[wagenet]

Rebased again. If this still fails, it's relevant though if we replace the router then it won't be :D

[johanrd]

FWIW: I tried to reproduce by rebasing / adding the test on main, and all tests are green there: johanrd#7

Not sure if it we should rebase properly and actually add the test, or just close the PR.

[kategengler]

Since it passes and we're actively working on replacing the router, I don't think we need to add the test.

Thanks for discovering that info, @johanrd!