feat(typescript): typescript 4.5 support #3205

rwaskiewicz · 2022-01-13T21:37:09Z

Pull request checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)
Build (npm run build) was run locally and any changes were pushed
Unit tests (npm test) were run locally and passed
E2E Tests (npm run test.karma.prod) were run locally and passed
Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

What is the current behavior?

We currently support v4.3 of TypeScript

GitHub Issue Numbers:

Relates to bug: Unknown compiler option 'useUnknownInCatchVariables'. #3170

What is the new behavior?

This PR adds support for TypeScript 4.5. I originally intended to only bump Stencil to v4.4, but there were some changes to a private TypeScript API Stencil uses (and shouldn't, more on that later) in TS v4.4 that were reverted in v4.5 (again, more on that later). Below is a summary of each commit by name (should I rebase this and mess up the SHA links)

chore(dependencies): upgrade typescript to v4.4.4

Version bump us to v4.4.4 - this was before I realized we'd have a problem with 4.4. This commit will later be superseded by the upgrade to 4.5.

update client browser patching

update client-patch-browser.ts patching of performance.mark() &
performance.measure() to be ignored by the type checker. this patch
allows stencil to support Safari 10; since we don't have access to a
non-existent performance buffer and changing the return value for either
of these functions could be considered a breaking change, defer altering
the (non-compliant) functions until stencil drops support for Safari 10

remove non-compliant window.location.reload() arguments

reload has no parameter, although Firefox supports a non-standard
'forceGet' boolean parameter for bypassing its cache. for all other
browsers, this is otherwise ignored. these arguments are used in the
context of stencil's dev-server (IE are only used at dev time). given
these two statements, the risk of a truly breaking change is deemed
minimal here

update fn signature of mock-doc's customElementRegistry#whenDefined

set the return type of whenDefined to be
Promise<CustomElementConstructor> over Promise<void> to meet new
type checking expectations with TS v4.4. this leads to a small
functional change in that the return value for this function when the
internal registry contains an entry for a given tagName, the function
now returns the constructor associated with the tag name, rather than
void.

update mock-doc performance function signatures

update the function signatures of mock-doc's performance implementation
by declaring the return type on mark() and measure() to align with
the Performance interface, but continue to return undefined,
ignorning the type checker errors.

stencil's stubs for these functions assume access to underlying browser
api's (namely a performance buffer). given that older browsers may not
support this functionality and implementating a mock version of these
functions falls well outside the scope of a typescript update, simply
ignore the errors.

we take this approach, rather than:

// @ts-ignore
mark(): void { /* no-op */ }

as stencil can (and does) instantiate an instance of MockPerformance
and attempts to assign it to variable(s) that are of type
Performance. this circumvents the type checker (although stencil was
lying to it already) to prevent additional // @ts-ignore pragmas

this may result in typing errors for consumers, however the actual
values shall remain the same.

window - add ts-ignore to handle mock-window & window type diffs

In various parts of the codebase, Window is used as a typing when the actual concrete type is a MockWindow - this leads to some tricky spots where when the TS compiler sees these as two distinct, unrelated types, calling a MockWindow method on an object typed as Window leads to compiler errors/adding @ts-ignore pragmas to the code. Created a ticket for us to revisit this

useUnknownInCatchVariables

enable useUnknownInCatchVariables in all TSConfig files, with the
exception of the main tsconfig.json. Although flipping that bit was
originally going to be a part of the TS 4.4 upgrade, our decision to
upgrade from 4.3 -> 4.5 directly increased scope a bit. That and the
fact there are ~90 places where we want to rework the code, I feel more
comfortable splitting that off into a separate effort

[PAUSE] Stop current line of work, look into TS 4.5 level of effort

i'm concerned about the matchFile call and how the method changes for
v4.4, then is reverted in v4.5. based on the conversations/issues in GH
surrounding these changes, I'm strongly considering moving directly to
v4.5. pause here to investigate those changes

TO BE CLEAR - Stencil needs to move off this API. I have a story in the backlog for us to do this

TS 4.5

update karma-typescript with monounity/karma-typescript@f4cdc43 - this is necessary due to internal changes made in TypeScript 4.5.

update TypeScript to v4.5.4

remove the temporary hack during the TS v4.4 upgrade for matchFile
call to succeed.

update single unit test that was failing following the upgrade.

update typescript import statements that were broken as a result of the
upgrade to handle AssertClauses. move typescript calls to use factory
methods

Does this introduce a breaking change?

Yes
No - I'm following the same guidance we have for previous upgrades here. We treat TS upgrades as 'minor'/'non-breaking', just as the TS team does.
TypeScript 4.4 breaking changes
TypeScript 4.5 breaking changes

Because there's a little bit of a chance there could be a less than smooth upgrade here, I wanna hold off on adding this to the Stencil v2.13 release, and slate this for v2.14

Testing

In addition to unit/karma tests helping us out here, I also installed this branch's generated tarball (npm run build && npm pack) in Ionic Framework's core module, then built Framework/ran its tests. For better or worse, the source of most breaking changes would come from the TS upgrade itself (rather than changes we made to accommodate the upgrade, since there are a few ts-ignore pragmas in here)

update client-patch-browser.ts patching of `performance.mark()` & `performance.measure()` to be ignored by the type checker. this patch allows stencil to support Safari 10; since we don't have access to a non-existent performance buffer and changing the return value for either of these functions could be considered a breaking chamge, defer altering the (non-compliant) functions until stencil drops support for Safari 10

`reload` has no parameter, although Firefox supports a non-standard 'forceGet' boolean parameter for bypassing its cache. for all other browsers, this is otherwise ignored. these arguments are used in the context of stencil's dev-server (IE are only used at dev time). given these two statements, the risk of a truly breaking change is deemed minimal here

set the return type of `whenDefined` to be `Promise<CustomElementConstructor>` over `Promise<void>` to meet new type checking expectations with TS v4.4. this leads to a small functional change in that the return value for this function when the internal registry contains an entry for a given `tagName`, the function now returns the constructor associated with the tag name, rather than void.

@ts-ignore

update the function signatures of mock-doc's performance implementation by declaring the return type on `mark()` and `measure()` to align with the `Performance` interface, but _continue to return undefined_, ignorning the type checker errors. stencil's stubs for these functions assume access to underlying browser api's (namely a performance buffer). given that older browsers may not support this functionality and implementating a mock version of these functions falls well outside the scope of a typescript update, simply ignore the errors. we take this approach, rather than: ```typescript // @ts-ignore mark(): void { /* no-op */ } ``` as stencil can (and does) instantiate an instance of `MockPerformance` and attempts to assign it to variable(s) that are of type `Performance`. this circumvents the type checker (although stencil was lying to it already) to prevent additional `// @ts-ignore` pragmas this may result in typing errors for consumers, however the actual values shall remain the same.

enable `useUnknownInCatchVariables` in all TSConfig files, with the exception of the main `tsconfig.json`. Although flipping that bit was originally going to be a part of the TS 4.4 upgrade, our decision to upgrade from 4.3 -> 4.5 directly increased scope a bit. That and the fact there are ~90 places where we want to rework the code, I feel more comfortable splitting that off into a separate effort

i'm concerned about the `matchFile` call and how the method changes for v4.4, then is reverted in v4.5. based on the conversations/issues in GH surrounding these changes, I'm strongly considering moving directly to v4.5. pause here to investiage those changes

update karma-typescript with monounity/karma-typescript@f4cdc43 update TypeScript to v4.5.4 remove the temporary hack during the TS v4.4 upgrade for `matchFile` call to succeed. update single unit test that was failing following the upgrade. update typescript import statements that were broken as a result of the upgrade to handle `AssertClause`s. move typescript calls to use factory methods

ltm · 2022-01-27T14:37:42Z

src/mock-doc/custom-element-registry.ts

    tagName = tagName.toLowerCase();

    if (this.__registry != null && this.__registry.has(tagName) === true) {
-      return Promise.resolve();
+      return this.__registry.get(tagName).cstr;


I'm not completely sure what the type of cstr is, but assuming it's CustomElementConstructor I would expect this to still return a promise:

Suggested change

return this.__registry.get(tagName).cstr;

return Promise.resolve<CustomElementConstructor>(this.__registry.get(tagName).cstr);

Good call - technically it may not be a promise. Updated in b365de1

rwaskiewicz added 9 commits January 12, 2022 17:02

chore(dependencies: upgrade typescript to v4.4.4

f0a04cb

window - add ts-ignore to handle mock-window & window type diffs

28269ab

rwaskiewicz changed the title ~~chore(typescript): typescript 4.5 support~~ feat(typescript): typescript 4.5 support Jan 13, 2022

rwaskiewicz mentioned this pull request Jan 20, 2022

chore(typescript): enable useUnknownInCatchVariables in core #3211

Merged

14 tasks

rwaskiewicz marked this pull request as ready for review January 24, 2022 18:49

rwaskiewicz requested a review from a team January 24, 2022 18:49

Merge branch 'main' into ts4.5

8520a83

ltm reviewed Jan 27, 2022

View reviewed changes

return Promise for mock-doc CE registry hit instead of bare ctor

b365de1

ltm approved these changes Jan 27, 2022

View reviewed changes

Merge branch 'main' into ts4.5

5b376e3

rwaskiewicz merged commit 806012e into main Jan 27, 2022

rwaskiewicz deleted the ts4.5 branch January 27, 2022 18:47

jcfranco mentioned this pull request Feb 17, 2022

build(deps): bump @stencil/core from 2.13.0 to 2.14.0 Esri/calcite-design-system#4140

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(typescript): typescript 4.5 support #3205

feat(typescript): typescript 4.5 support #3205

rwaskiewicz commented Jan 13, 2022 •

edited

ltm Jan 27, 2022

rwaskiewicz Jan 27, 2022

	return this.__registry.get(tagName).cstr;
	return Promise.resolve<CustomElementConstructor>(this.__registry.get(tagName).cstr);

feat(typescript): typescript 4.5 support #3205

feat(typescript): typescript 4.5 support #3205

Conversation

rwaskiewicz commented Jan 13, 2022 • edited

Pull request checklist

Pull request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Testing

ltm Jan 27, 2022

Choose a reason for hiding this comment

rwaskiewicz Jan 27, 2022

Choose a reason for hiding this comment

rwaskiewicz commented Jan 13, 2022 •

edited