Migrate navigation folder to typescript #1202
Conversation
src/navigation/navigation-events.ts
Outdated
| obj.currentTarget && | ||
| obj.currentTarget.href && | ||
| obj.preventDefault | ||
| (obj as MouseEvent).currentTarget && |
There was a problem hiding this comment.
I'm open to a better typescript solution that avoids repeated typecasting inside of this if statement. VS Code type checking didn't infer that obj was a MouseEvent after the two previous if statements. Type casting seemed to be the simplest approach that avoided runtime changes
There was a problem hiding this comment.
depending on the level of "not wanting to change the runtime" you want, you could consider using the in operator for this, which for reasons unknown to me* make TS way happier:
* I suspect it's due to the difference between
const car = {sold: false, fancy: undefined}
// doesn't enter if statement
if(car.sold)...
if(car.fancy)...
// does enter if statement
if('sold' in car)...
if('fancy' in car)...but am not actually sure on whether that's the real reason or not.
There was a problem hiding this comment.
👍 I pushed a commit with this change
src/navigation/navigation-events.ts
Outdated
| window.location.hash = destination.hash; | ||
| } else if (current.host !== destination.host && destination.host) { | ||
| if (process.env.BABEL_ENV === "test") { | ||
| // @ts-ignore |
There was a problem hiding this comment.
In some of the single-spa tests, the function signature for navigateToUrl changes to have an object as the return type. Since that is never the case in the dev or prod builds of single-spa, I opted not to include that possibility in the return type for navigateToUrl. This resulted in the need for a ts-ignore here. I'm open to other options - a test-only return type is unusual and I thought a ts-ignore was warranted.
There was a problem hiding this comment.
I kind of like using @ts-expect-error [text here] so that if the implementation ever changes and it starts working again, TS will be helpful and tell you to remove this comment. Not a big deal though, just a personal preference.
| function urlReroute() { | ||
| reroute([], arguments); | ||
| function urlReroute(evt: HashChangeEvent | PopStateEvent) { | ||
| reroute([], [evt]); |
There was a problem hiding this comment.
This is a runtime change that I hope doesn't cause any changes to behavior for single-spa users. I made the change so that type checks were stricter.
The urlReroute function is added as an event listener to the window hashchange and popstate events. According to MDN docs, there is only one argument passed to hashchange and popstate events, so I don't think this change is safe.
| try { | ||
| evt = new PopStateEvent("popstate", { state }); | ||
| } catch (err) { | ||
| // IE 11 compatibility https://github.com/single-spa/single-spa/issues/299 |
There was a problem hiding this comment.
I removed this IE-specific workaround
| const current = parseUri(window.location.href); | ||
| const destination = parseUri(url); | ||
| const current = new URL(window.location.href); | ||
| const destination = new URL(url, window.location.href); |
There was a problem hiding this comment.
IE10 and/or 11 didn't support new URL() to correctly parse all strings. Since single-spa@7 doesn't support IE, I've removed the parseUri function in favor of new URL.
I am not totally sure that the behavior is the same for the two implementations, but I think it should be. The main edge cases to consider are relative urls - I believe using window.location.href as the base url will be equivalent to creating an anchor element with a relative url for the href
| } | ||
|
|
||
| const unloadPromises = appsToUnload.map(toUnloadPromise); | ||
| const unloadPromises: Promise<InternalApplication>[] = |
There was a problem hiding this comment.
Once the lifecycles folder is migrated to typescript, the unloadPromises variable should have automatically-inferred types. Since that hasn't happened yet, though, I've put the types manually
| } | ||
| ); | ||
| ) | ||
| .then(finishUpAndReturn); |
There was a problem hiding this comment.
I think this might be a fix for a bug that was accidentally introduced in single-spa@6 with the profiler code. The finishUpAndReturn sets the resolved value of the promise chain, so it should be at the end.
| const mountPromises = appsToMount | ||
| .filter((appToMount) => appsToLoad.indexOf(appToMount) < 0) | ||
| const mountPromises: Promise<InternalApplication>[] = appsToMount | ||
| .filter((appToMount) => !appsToLoad.includes(appToMount)) |
There was a problem hiding this comment.
Another small change to remove IE support - using Array.prototype.includes rather than Array.prototype.indexOf
|
|
||
| if (extraProperties) { | ||
| assign(result.detail, extraProperties); | ||
| Object.assign(result.detail, extraProperties); |
There was a problem hiding this comment.
Native assign rather than IE workaround
| "emitDeclarationOnly": true, | ||
| "declaration": true, | ||
| "lib": ["dom", "es5", "es2015"] | ||
| "lib": ["dom", "es5", "es2020"] |
There was a problem hiding this comment.
Needed for Promise.prototype.finally
frehner
left a comment
frehner
left a comment
There was a problem hiding this comment.
Really only looked at a small portion of this PR, but generally seems like you have a good handle on this.
src/navigation/navigation-events.ts
Outdated
| obj.currentTarget && | ||
| obj.currentTarget.href && | ||
| obj.preventDefault | ||
| (obj as MouseEvent).currentTarget && |
There was a problem hiding this comment.
depending on the level of "not wanting to change the runtime" you want, you could consider using the in operator for this, which for reasons unknown to me* make TS way happier:
* I suspect it's due to the difference between
const car = {sold: false, fancy: undefined}
// doesn't enter if statement
if(car.sold)...
if(car.fancy)...
// does enter if statement
if('sold' in car)...
if('fancy' in car)...but am not actually sure on whether that's the real reason or not.
src/navigation/navigation-events.ts
Outdated
| window.location.hash = destination.hash; | ||
| } else if (current.host !== destination.host && destination.host) { | ||
| if (process.env.BABEL_ENV === "test") { | ||
| // @ts-ignore |
There was a problem hiding this comment.
I kind of like using @ts-expect-error [text here] so that if the implementation ever changes and it starts working again, TS will be helpful and tell you to remove this comment. Not a big deal though, just a personal preference.
No description provided.