Add typed lint checks and fix promise related things #1081
Conversation
| "@typescript-eslint/no-redundant-type-constituents": "warn", | ||
|
|
||
| // ideally these would be an error because it does catch bugs | ||
| // but it also requires a lot of code change right now |
There was a problem hiding this comment.
Originally I didnt disable all of these but i had 100s of files of change and decided to start back over with a minimal set and we can do different PRs to enable these later
| @@ -31,7 +31,7 @@ export async function getWebsite( | |||
|
|
|||
| try { | |||
| const result = await fetch(url); | |||
| return result.json(); | |||
| return await result.json(); | |||
There was a problem hiding this comment.
Gives better stack traces
| @@ -115,7 +115,7 @@ describe(createClient, () => { | |||
| "makeConjureContext", | |||
| ); | |||
|
|
|||
| metadataCacheClient( | |||
| void metadataCacheClient( | |||
There was a problem hiding this comment.
Signals to the linter that we know this returns a promise and we do not care
| ontologyRid.then((ontologyRid) => { | ||
| if (!ontologyRid.startsWith("ri.")) { | ||
| // FIXME this promise is not await so this just shows up as an unhandled promise rejection | ||
| throw new Error("Invalid ontology RID"); |
There was a problem hiding this comment.
@nihalbhatnagar have a follow up for you
There was a problem hiding this comment.
Ah just need a .catch here?
There was a problem hiding this comment.
I think you probably don't want to throw because it will just end up as an unhandled promise. Maybe we want to set ontologyRid = ontologyRid.then(... and that way when we do end up using it it will pop up through a call stack?
We could maybe add a console.error() for the first time its hit too?
There was a problem hiding this comment.
I see what you mean, this makes sense to me
| latestPointPromise.then( | ||
| latestPoint => this.lastFetchedValue = latestPoint, | ||
| // eslint-disable-next-line no-console | ||
| err => void console.error(err), |
There was a problem hiding this comment.
Not handling the error case means we have an unhandled promise rejection which can be swallowed
| else window.location.assign(x); | ||
| if (useHistory) { | ||
| window.history.replaceState({}, "", x); | ||
| return; |
There was a problem hiding this comment.
Auto fixer did this when I saved a different change. I kinda like the return void window... syntax better but there are two different promise rules and that causes an issue with another one since .assign isn't returning a promise, it returns void and so the void window.history... is "redundant". But its not cause we need it for the async function. Sigh. So i left this auto fixer as is.
| @@ -55,7 +55,7 @@ export async function publishPackages( | |||
| ); | |||
|
|
|||
| // need to produce the output to save for future run of create tag/release | |||
| fs.writeFile( | |||
| await fs.writeFile( | |||
| @@ -248,7 +248,7 @@ export function FoundryWidgetVitePlugin(_options: Options = {}): Plugin { | |||
| res.statusCode = settingsResponse.status; | |||
| res.statusMessage = | |||
| `Unable to set widget settings in Foundry: ${settingsResponse.statusText}`; | |||
| settingsResponse.text().then((err) => { | |||
| await settingsResponse.text().then((err) => { | |||
There was a problem hiding this comment.
Prevents unhandled rejection + improves stack trace
| @@ -669,6 +669,7 @@ function getPluginTransformHook( | |||
| typeof plugin.transformIndexHtml === "object" | |||
| && "transform" in plugin.transformIndexHtml | |||
| ) { | |||
| // eslint-disable-next-line @typescript-eslint/no-deprecated | |||
| return plugin.transformIndexHtml.transform; | |||
There was a problem hiding this comment.
I think in general its good for it to be an error when we use deprecated types but I dont want to deep research vite plugins to understand if there is a consequence to this right now so we will save it as is.
| @@ -27,7 +27,7 @@ | |||
| "tsconfig.json" | |||
| ], | |||
| "outputs": [], | |||
| "dependsOn": ["//#global-eslint-config", "//#dprint"] | |||
| "dependsOn": ["//#global-eslint-config", "//#dprint", "typecheck"] | |||
| }, | |||
|
|
|||
There was a problem hiding this comment.
Gotta be sure we have compiled all of our dependencies
No description provided.