Configuring a route with "redirect" property causes BlueBird warnings

[ry8806]

I'm submitting a bug report

• Library Version:
  1.0.1

Please tell us about your environment:

• Operating System:
  Windows 10

• Browser:
  all

• Language:
  all

Current behavior:
specifying "redirect" property in a route configuration, causes BlueBird warnings about "Warning: a promise was rejected with a non-error: [object Object]"

This comes from the router method:

  function _buildNavigationPlan(instruction, forceLifecycleMinimum) {
    var prev = instruction.previousInstruction;
    var config = instruction.config;
    var plan = {};

    if ('redirect' in config) {
      var redirectLocation = _resolveUrl(config.redirect, getInstructionBaseUrl(instruction));
      if (instruction.queryString) {
        redirectLocation += '?' + instruction.queryString;
      }

      return Promise.reject(new Redirect(redirectLocation));
    }

I tired creating a gist given the link in the Issue Template, however i can't replicate it in a Gist - i'm assuming the bluebird promises library isn't included in gists?

Expected/desired behavior:
Not to create a warning in the console everytime that specific route is redirected

• What is the motivation / use case for changing the behavior?
  Less warnings in the console

I understand that this is a BlueBird "quirk", however I'm posing the question as to whether Aurelia adjusts its use of BlueBird to stop these warnings? or if we need to go to BlueBird and propose a "don't warn me when a promise is rejected with a non-error" flag?

[renathoaz]

The same warning is thrown for:

config.mapUnknownRoutes({ redirect:'home'});

is there any way to suppress it ?

[jagonalez]

If you don't want these to show then you need to disable them in your bluebird config:

If you're using Aurelia CLI you can change it within the main.js file.

Promise.config({
  warnings: false
});

[ry8806]

@jagonalez I'm aware of this, thanks

This "silence all warnings" flag isn't the solution, by disabling ALL warnings (for the issue mentioned above) I might be losing out on other warnings that it may be trying to warn me about

[jagonalez]

@ry8806 I agree - it's not ideal. In our app we're using environment variables to turn on warnings only for debugging.

Looking at the w3 specification Promise.reject should return an Error object:

The w3 specification for Promises recommends that reject return an instance of Error. https://www.w3.org/2001/tag/doc/promises-guide#reasons-should-be-errors

Promise rejection reasons should always be instances of the ECMAScript Error type, just like synchronously-thrown exceptions should always be instances of Error as well.

Bluebird's docs explain why:
https://github.com/petkaantonov/bluebird/blob/master/docs/docs/warning-explanations.md

Due to a historic mistake in JavaScript, the throw statement is allowed to be used with any value, not just errors, and Promises/A+ choosing to inherit this mistake, it is possible to reject a promise with a value that is not an error.

The only way to stop the console warnings is to change the code to return an Error or not use Promise.reject for redirection.

@EisenbergEffect - What are your thoughts?

[indfnzo]

Updates on this? This has been bugging me for a while now.

Is there really nothing else we can do except disable all warnings?

[cornillemichiel]

I get why the warning is thrown, and valid, but I don't see why we'd reject a promise to do a redirect.

If we HAVE to use promise rejection, couldnt this issue be solved by inheriting redirects from Error? Or adding the needed property stubs (.stack and .message) to the redirect class?

[Kukks]

I get this is a low profile issue but could we get an official response on whether we should expect this to be handled or if the only workaround is to disable warning?

[eric-914]

I'm now hitting this problem, and would like to know if there's any status change on how to fix it?

[davismj]

@Kukks for the record, the problem is just a warning being logged to the console?

I think a good, viable long term fix would be to allow resolving a redirect as well. I agree that while the reject notation half makes sense, it half doesn't. PRs welcome if someone wants to take a stab at that.

[fkleuver]

@davismj I've been digging into the router recently and noticed a few places where promises aren't being returned from one internal method to the next.

One example in the app-router, _queueInstruction calls _dequeueInstruction but doesn't return it.

Another one is registerViewPort which calls _dequeueInstruction at the bottom.

So what happens to the promise that's returned by _dequeueInstruction in both cases?

I can't help but wonder if this is intended for some kind of asynchrony, or if this was overlooked at some point? I'm fairly sure that this sort of thing is what causes the promise warnings.

[davismj]

@fkleuver as I didn't write the code and there aren't a lot of comments, I can't say for sure whether or not the highly async architecture was the right way to go. I've noticed the same things and they seem like a liability to me as well, but they're (a) deep and (b) pervasive, so I'm not in any place to start messing with them.

That said, as far as I understand, this particular issue is caused when aurelia internals reject() Redirects to cancel navigation instead of resolving. This is wrong in the strictest interpretation of promises, in which reject() is the async version of a throw, and bluebird tells anyone who will listen.

[Alexander-Taran]

Can we change this?
Or we can't?
Should we?

Or just keep in mind for vNext?

[fkleuver]

As davismj pointed out, it seems my earlier comment isn't applicable to this specific issue. I'd go with his suggestion to allow redirects to be resolved as being the point of action here.
I seem to recall someone was working on that in another PR but I can't find it right now.

I might take a stab at it if no one else is working on it. Could you double check as you're the one who's on top of the issues? :)

[Alexander-Taran]

I remember too.. there were an issue where a question was asked..
do we really want to fix a warning? when nothing is really broken?
Is it really that easy to distract developers?

[fkleuver]

Technically it's an uncaught error, and that's certainly not a good thing.

[Alexander-Taran]

Technically-shmecnically
It works?
don't change it!
(-:
How's that for an argument?

[davismj]

@fkleuver No one is working on it as far as I know.

[rluba]

The fix introduced in 487c33d causes a much more serious error when redirecting in a sub-router config: TypeError: Cannot read property 'childRouter' of undefined is thrown because determineWhatToLoad(…) tries to interpret the Redirect plan as a regular plan because the rejection needs to be handled in more places than just the one checked in 487c33d.

[stefan505]

I concur with @rluba , after doing a "clean" install this morning referencing [email protected] (since Feb 2018), I got [email protected], which is causing the TypeError: Cannot read property 'childRouter' of undefined error. Manually installing [email protected] fixed it though.

[Kukks]

Same boat, broke our prodcution builds since we get rid of package lock json file due to other weird crazy errors from aurelia when we leave it in

…

On Tue, Jun 19, 2018 at 10:37 AM Stefan Buys ***@***.***> wrote: I concur with @rluba <https://github.com/rluba> , after doing a "clean" install this morning referencing ***@***.*** (since Feb 2018), I got ***@***.***, which is causing the TypeError: Cannot read property 'childRouter' of undefined error. Manually installing ***@***.*** fixed it though. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#480 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABu-_mjzjdrVBxsMedsiu28RsF3Guo-xks5t-Lg0gaJpZM4MjrVJ> .

[fkleuver]

Yep agreed, #603 should be merged asap. Sorry for breaking your apps!

[davismj]

It'll be getting released today.

[ricardograca]

Shouldn't this be reopened since #597 was reverted?