Open matthewcorven opened 4 years ago
Ping @davismj
If i understand correctly, returning a Promise
is incorrect, since it's not what the history navigate returns
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>
.
Thanks for the initial investigation @davismj
@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.":
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.
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?
@Calabonga let me have a look at this. From what I remember, the signature was wrong, and boolean was a fix
@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.
@bigopon , @matthewcorven Thanks for your quick answers, but...
@Calabonga I think this is the best-matching signature, but ultimately I'm looking to @davismj and @bigopon to validate.
@bigopon I think so too! Thanks. I'll be waiting.
@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 https://github.com/aurelia/router/issues/648#issuecomment-582686325, 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 😓
Thanks. I'll try to refactor my logic yet.
Any update @Calabonga ? CC @bigopon
Promise<PipelineResult | boolean>
, thenNavigationResult
and most recentlyboolean
(version 1.7.1).boolean
.https://github.com/aurelia/router/blob/893b768f01aea842ee57db4222e66aa572f24404/dist/aurelia-router.d.ts#L300