navigateToRoute return signature change - no longer a Promise?

[matthewcorven]

• Overview of the Issue: router navigateToRoute() previously has a return signature type of Promise<PipelineResult | boolean>, then NavigationResult and most recently boolean (version 1.7.1).
• Motivation or Use Case:
  • Upgrading from earlier versions to latest results in build failure in projects enforcing tslint rule "await-promise".
  • Code relying upon navigateToRoute as an awaitable function require refactoring
• Library Name and Version(s): aurelia-router, router.ts version 1.7.1 (current)
• Browsers and Operating System: N/A (build issue)
• Reproduce the Error: Build an application with lines below using aurelia-router version 1.7.1 and tslint "await-promise" rule turned on
• Related Issues: None found
• Suggest a Fix: Revert return signature change, or explain reason for change to return type boolean.

router/dist/aurelia-router.d.ts

Line 300 in 893b768

navigateToRoute(route: string, params?: any, options?: NavigationOptions): boolean;

[EisenbergEffect]

Ping @davismj

[bigopon]

If i understand correctly, returning a Promise is incorrect, since it's not what the history navigate returns

[davismj]

The typing is in fact wrong. After quickly tracing through, I want to say it should be boolean | Promise<PipelineResult> but it might be boolean | Promise<boolean | PipelineResult>.

[matthewcorven]

Thanks for the initial investigation @davismj

[matthewcorven]

@davismj @EisenbergEffect I'd be happy to submit a PR for this. I'm not sure where the CLA is that I need to sign. The CONTRIBUTING.md in this project sends me to:

http://goo.gl/forms/dI8QDDSyKR

Which says "Sorry, the file you have requested does not exist.":

[EisenbergEffect]

Please feel free to submit a PR :) When you submit the PR, Github will check our CLA sig list and prompt you with the proper way to sign. Sorry about the bad link. I thought I had that updated everywhere after we switched over to CLA Assistant. I'll get that fixed soon.

[Calabonga]

Hi, and again...

Aurelia version:

> au -v
Local aurelia-cli v1.2.3

method this.router.navigateToRoute and this.router.navigate are return both a Promise not the boolean :(
but:

/**
* Navigates to a new location corresponding to the route and params specified. Equivallent to [[Router.generate]] followed
* by [[Router.navigate]].
*
* @param route The name of the route to use when generating the navigation location.
* @param params The route parameters to be used when populating the route pattern.
* @param options The navigation options.
*/
	navigateToRoute(route: string, params?: any, options?: NavigationOptions): boolean;

If the bug is fixed how i can update aurelia-router.d.ts?

[bigopon]

@Calabonga let me have a look at this. From what I remember, the signature was wrong, and boolean was a fix

[matthewcorven]

@bigopon The change I had proposed for this I eventually abandoned because @davismj indicated that he had identical changes (as a Promise) in a local branch that hadn't yet been pushed, which he would carry forward.

[Calabonga]

@bigopon , @matthewcorven Thanks for your quick answers, but...

1. What's the correct type for return in fact?
2. When will I can download the correct version?

[matthewcorven]

@Calabonga I think this is the best-matching signature, but ultimately I'm looking to @davismj and @bigopon to validate.

router/src/interfaces.ts

Line 323 in 1725837

export type NavigationResult = boolean | Promise<PipelineResult | boolean>;

[Calabonga]

@bigopon I think so too! Thanks. I'll be waiting.

[bigopon]

@matthewcorven @Calabonga @davismj on the surface, boolean is the sufficient return type as it's the contract that History guarantees. However, the implementation of History doesn't conform this, and actually returns a different type: it's eventually: Promise<PipelineResult> if the pipeline ran successfully, or Promise<void> if there was an error in the pipeline. So final typing could be something along this line, I believe.

export type NavigationResult = boolean | Promise<PipelineResult | void>;

It's not too far off compared to what we have at the moment in the comment #648 (comment), so I think we can keep it in the current shape for now. We could refactor more, but there's concern that big refactoring without major semver change would destabilize the router, while providing not big enough value to justify it.

We will still fix critical bug though. So, ... sorry for the issue 😓

[Calabonga]

Thanks. I'll try to refactor my logic yet.

[matthewcorven]

Any update @Calabonga ?
CC @bigopon