chore(typescript): enable useUnknownInCatchVariables in core #3211
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
14 tasks
ltm
reviewed
Jan 25, 2022
| throw realpathErr; | ||
| } catch (realpathErr: unknown) { | ||
| if (realpathErr instanceof Object && 'code' in realpathErr) { | ||
| // @ts-ignore: we've determined 'code' is in the prototype chain, but TS isn't happy about the access |
There was a problem hiding this comment.
FYI: This is because in isn't a type guard: microsoft/TypeScript#21732. One option would be to implement a type guard function for NodeJS.ErrnoException:
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/node/globals.d.ts#L183-L188
ltm
reviewed
Jan 25, 2022
| @@ -139,9 +139,12 @@ export const createCustomResolverSync = ( | |||
| const fsFilePath = normalizeFsPath(p); | |||
| try { | |||
| return sys.realpathSync(fsFilePath); | |||
There was a problem hiding this comment.
Unrelated to your changes... does this even throw an exception? According to the interface of sys.realpathSync() it returns a CompilerSystemRealpathResults and doesn't throw an exception:
stencil/src/declarations/stencil-public-compiler.ts
Lines 1032 to 1035 in b00585f
There was a problem hiding this comment.
It shouldn't, although I think the problem here is that we can't guarantee implementations on that interface won't throw. Perhaps this is just a bit of defensive programming?
ltm
approved these changes
Jan 25, 2022
this commit enables the 'useUnknownInCatchVariables' flag in the core compiler's tsconfig.json file. the remainder of the changes related directly to errors emitted by the TypeScript compiler as a result of this change. the most common change in this commit is updating a `catch` block that consists of a single call to stencil's `catchError` function. because `catchError` may theoretically accept a value or object that does not conform to the `Error` interface, we annotate many of the error variables declared in the `catch` block as type `any`, in a good-faith effort not to lose any useful information that we would otherwise turn into a `Diagnostic`. Using a type guard or other type narrowing mechanism would be feasible if any value that is not of type `Error` were logged in some other way. However in an attempt to not pollute the codebase with two different logging paths, we accept the ambiguity of `catchError` and catching errors of type `any` for the time being.
rwaskiewicz
force-pushed
the
useUnknownInCatchVars
branch
from
b88604b
to
f223d61
Compare
rwaskiewicz
added a commit
that referenced
this pull request
Mar 2, 2022
this commit is a quick fix for the browser-compile tests that currently do not compile after setting `useUnknownInCatchVariables` in #3211. This went unnoticed as this application is not currently type checked by our CI process. In order to get the application to compile, we type the error as `any` for now, with the intent of properly narrowing it in STENCIL-371 (and adding proper CI checks there as well). this commit only unblocks next monday's release.
14 tasks
rwaskiewicz
added a commit
that referenced
this pull request
Mar 2, 2022
this commit is a quick fix for the browser-compile tests that currently do not compile after setting `useUnknownInCatchVariables` in #3211. This went unnoticed as this application is not currently type checked by our CI process. In order to get the application to compile, we type the error as `any` for now, with the intent of properly narrowing it in STENCIL-371 (and adding proper CI checks there as well). this commit only unblocks next monday's release.
14 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull request checklist
Please check if your PR fulfills the following requirements:
npm run build) was run locally and any changes were pushednpm test) were run locally and passednpm run test.karma.prod) were run locally and passednpm run prettier) was run locally and passedPull request type
Please check the type of change your PR introduces:
What is the current behavior?
Errors declared in
catchblocks are implicitly typed asanyGitHub Issue Number: N/A
What is the new behavior?
this commit enables the 'useUnknownInCatchVariables' flag in the core
compiler's tsconfig.json file. the remainder of the changes related
directly to errors emitted by the TypeScript compiler as a result of
this change.
the most common change in this commit is updating a
catchblock thatconsists of a single call to stencil's
catchErrorfunction. becausecatchErrormay theoretically accept a value or object that does notconform to the
Errorinterface, we annotate many of the errorvariables declared in the
catchblock as typeany, in a good-faitheffort not to lose any useful information that we would otherwise turn
into a
Diagnostic. Using a type guard or other type narrowingmechanism would be feasible if any value that is not of type
Errorwere logged in some other way. However in an attempt to not pollute the
codebase with two different logging paths, we accept the ambiguity of
catchErrorand catching errors of typeanyfor the time being.Does this introduce a breaking change?
Testing
The TS Compiler was run over the codebase, to ensure no type-safety issues. Admittedly it's not the most comprehensive, but covers most of the cases throughout this patch.
Other information
Do not merge please. Relies on #3205