Migrate lifecycles folder to typescript #1210
Conversation
.eslintrc
Outdated
| "parser": "@babel/eslint-parser", | ||
| "parser": "@typescript-eslint/parser", | ||
| "parserOptions": { | ||
| "lib": ["dom", "es5", "es2020"] |
There was a problem hiding this comment.
I was surprised that I had to specify this option because I thought it would inherit it from the tsconfig.json. Apparently it did not.
There was a problem hiding this comment.
Apparently, you can also set the project setting to point to the tsconfig.json. I would've expected it to automatically detect tsconfig.json at the same level as the .eslintrc too.
| export function flattenFnArray( | ||
| appOrParcel: LifeCycles<unknown>, | ||
| lifecycle: "bootstrap" | "mount" | "update" | "unmount" | "unload", | ||
| isParcel: boolean |
There was a problem hiding this comment.
This is a runtime change that maybe fixes a bug? The objectType function looks for a runtime unmountThisParcel property, but flattenFnArray is being called with the user-defined result of the load promise, rather than with InternalApplication | InternalParcel.
Requiring the caller of flattenFnArray to specify whether it's a parcel or not eliminates that possible bug
| @@ -26,14 +33,14 @@ export function getProps(appOrParcel) { | |||
| customProps | |||
| ); | |||
| } | |||
| const result = assign({}, customProps, { | |||
| const result: SingleSpaProps = Object.assign({}, customProps, { | |||
There was a problem hiding this comment.
Switched to native Object.assign(), since we no longer support IE in single-spa@7
.eslintrc
Outdated
| "parser": "@babel/eslint-parser", | ||
| "parser": "@typescript-eslint/parser", | ||
| "parserOptions": { | ||
| "lib": ["dom", "es5", "es2020"] |
There was a problem hiding this comment.
Apparently, you can also set the project setting to point to the tsconfig.json. I would've expected it to automatically detect tsconfig.json at the same level as the .eslintrc too.
| if (appOrParcel.loadPromise) { | ||
| return appOrParcel.loadPromise; | ||
| if ((app as LoadedApp).loadPromise) { | ||
| return (app as LoadedApp).loadPromise; |
There was a problem hiding this comment.
The fact that TS can't infer this is... annoying.
src/lifecycles/prop.helpers.ts
Outdated
| unmountSelf(): Promise<AppOrParcel>; | ||
| } | ||
|
|
||
| export function getProps(appOrParcel: AppOrParcel): SingleSpaProps { |
There was a problem hiding this comment.
It might be useful if the return type here was SingleSpaProps & CustomProps, although I suppose the real utility would be if that was the type passed to the lifecycle functions itself (since CustomProps should be down-castable to the custom props an app expects.
src/lifecycles/prop.helpers.ts
Outdated
| @@ -26,14 +33,14 @@ export function getProps(appOrParcel) { | |||
| customProps | |||
| ); | |||
| } | |||
| const result = assign({}, customProps, { | |||
| const result: SingleSpaProps = Object.assign({}, customProps, { | |||
| name, | |||
| mountParcel: mountParcel.bind(appOrParcel), | |||
| singleSpa, | |||
| }); | |||
|
|
|||
| if (isParcel(appOrParcel)) { | |||
There was a problem hiding this comment.
It might be useful to redefine isParcel() as:
function isParcel(appOrParcel: AppOrParcel): appOrParcel is InternalParcelThis shouldn't requires any runtime changes, but it eliminates the need for a cast here. Just a thought for a future PR.
There was a problem hiding this comment.
I wasn't aware of the is appOrParcel is InternalParcel syntax - very cool. I've updated it.
No description provided.