Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): support TypeScript 4.8 #47038

Closed
wants to merge 3 commits into from
Closed

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Aug 4, 2022

Adds support for TypeScript 4.8 and resolves some issues that came up as a result of the update.

Most of the issues came from some changes in TypeScript where the decorators and modifiers properties were removed from most node types, and were combined into a single modifiers array. Since we need to continue supporting TS 4.6 and 4.7 until v15, I ended up creating a new ngtsc/ts_compatibility directory to make it easier to reuse the new backwards-compatible code.

@@ -42,6 +42,6 @@
"protractor": "file:../../node_modules/protractor",
"ts-node": "8.3.0",
"tslint": "5.18.0",
"typescript": "file:../../node_modules/typescript"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These integration tests had to be locked down to 4.7 temporarily, because the CLI needs to be updated.

@crisbeto crisbeto force-pushed the ts-4.8 branch 3 times, most recently from 569c43b to 40d2202 Compare August 4, 2022 10:55
@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer target: minor This PR is targeted for the next minor release labels Aug 4, 2022
@crisbeto crisbeto marked this pull request as ready for review August 4, 2022 11:10
@crisbeto crisbeto added the area: core Issues related to the framework runtime label Aug 4, 2022
@ngbot ngbot bot added this to the Backlog milestone Aug 4, 2022
@crisbeto
Copy link
Member Author

crisbeto commented Aug 5, 2022

@crisbeto crisbeto force-pushed the ts-4.8 branch 3 times, most recently from 0c3e2a8 to 92690f2 Compare August 8, 2022 11:33
@crisbeto
Copy link
Member Author

crisbeto commented Aug 8, 2022

I've reworked the PR based on the feedback.

@crisbeto
Copy link
Member Author

crisbeto commented Aug 8, 2022

@crisbeto
Copy link
Member Author

crisbeto commented Aug 8, 2022

@alxhub the feedback has been addressed.

Copy link
Contributor

@JiaLiPassion JiaLiPassion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@crisbeto crisbeto force-pushed the ts-4.8 branch 3 times, most recently from 8f4e648 to 619ab8c Compare August 9, 2022 16:01
Copy link
Member

@alxhub alxhub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we're doing this, I think we should have an integration test which installs TS 4.7 explicitly and verifies that our compiler still works correctly. We have that coverage now because the CLI is still using 4.7 so most of the tests are pinned, but we should have one that explicitly chooses to use 4.7.

packages/compiler-cli/src/ngtsc/core/src/compiler.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/metadata/src/util.ts Outdated Show resolved Hide resolved
* TODO(crisbeto): this is a backwards-compatibility layer for versions of TypeScript less than 4.7.
* We should remove it once we have dropped support for the older versions.
*/
export const updateTypeParameterDeclaration: Ts47UpdateTypeParameterDeclarationFn =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused by the types/logic here.

  1. Our compatibility layer should be exposing the TS 4.8 API shape, not the <= 4.7 shape. That is, we should be wrapping older TS versions to be compatible with the new signature, not the other way around.
  2. The condition PARSED_TS_VERSION >= 4.7 seems wrong - shouldn't that be <= in this context? Currently it lumps TS 4.7 and 4.8 together, which just seems wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pre-existing utility that I moved out here for consistency:

export function tsUpdateTypeParameterDeclaration(

@crisbeto
Copy link
Member Author

crisbeto commented Aug 9, 2022

The latest set of feedback has been addressed. The new code is in the last fixup commit.

Adds support for TypeScript 4.8 and resolves some issues that came up as a result of the update.

Most of the issues came from some changes in TypeScript where the `decorators` and `modifiers` properties were removed from most node types, and were combined into a single `modifiers` array. Since we need to continue supporting TS 4.6 and 4.7 until v15, I ended up creating a new `ngtsc/ts_compatibility` directory to make it easier to reuse the new backwards-compatible code.
@crisbeto
Copy link
Member Author

I've pushed another commit to update to the 4.8 RC. No other changes were necessary.

Copy link
Member

@alxhub alxhub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 this is looking really good. The compatibility layer is nice and clean, and should be maintainable while we have to deal with this split.

Let's talk on Monday about ts.getMutableClone as well.

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Aug 13, 2022
@crisbeto
Copy link
Member Author

Merge assistance: only the inclusive language check is failing on the name of one of the TSLint rule names that we aren't able to change.

@pkozlowski-opensource
Copy link
Member

This PR was merged into the repository by commit 31429ea.

crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Sep 1, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@angular/animations](https://github.com/angular/angular) | dependencies | minor | [`14.1.3` -> `14.2.0`](https://renovatebot.com/diffs/npm/@angular%2fanimations/14.1.3/14.2.0) |
| [@angular/common](https://github.com/angular/angular) | dependencies | minor | [`14.1.3` -> `14.2.0`](https://renovatebot.com/diffs/npm/@angular%2fcommon/14.1.3/14.2.0) |
| [@angular/compiler](https://github.com/angular/angular) | dependencies | minor | [`14.1.3` -> `14.2.0`](https://renovatebot.com/diffs/npm/@angular%2fcompiler/14.1.3/14.2.0) |
| [@angular/compiler-cli](https://github.com/angular/angular/tree/main/packages/compiler-cli) ([source](https://github.com/angular/angular)) | devDependencies | minor | [`14.1.3` -> `14.2.0`](https://renovatebot.com/diffs/npm/@angular%2fcompiler-cli/14.1.3/14.2.0) |
| [@angular/core](https://github.com/angular/angular) | dependencies | minor | [`14.1.3` -> `14.2.0`](https://renovatebot.com/diffs/npm/@angular%2fcore/14.1.3/14.2.0) |
| [@angular/forms](https://github.com/angular/angular) | dependencies | minor | [`14.1.3` -> `14.2.0`](https://renovatebot.com/diffs/npm/@angular%2fforms/14.1.3/14.2.0) |
| [@angular/platform-browser](https://github.com/angular/angular) | dependencies | minor | [`14.1.3` -> `14.2.0`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser/14.1.3/14.2.0) |
| [@angular/platform-browser-dynamic](https://github.com/angular/angular) | dependencies | minor | [`14.1.3` -> `14.2.0`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser-dynamic/14.1.3/14.2.0) |

---

### Release Notes

<details>
<summary>angular/angular</summary>

### [`v14.2.0`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#&#8203;1420-2022-08-25)

[Compare Source](angular/angular@14.1.3...14.2.0)

##### animations

| Commit | Type | Description |
| -- | -- | -- |
| [b96e571897](angular/angular@b96e571) | fix | fix stagger timing not handling params ([#&#8203;47208](angular/angular#47208)) |

##### common

| Commit | Type | Description |
| -- | -- | -- |
| [b380fdd59e](angular/angular@b380fdd) | feat | add a density cap for image srcsets ([#&#8203;47082](angular/angular#47082)) |
| [7ce497e5bc](angular/angular@7ce497e) | feat | add built-in Imgix loader ([#&#8203;47082](angular/angular#47082)) |
| [bff870db61](angular/angular@bff870d) | feat | add cloudflare loader ([#&#8203;47082](angular/angular#47082)) |
| [86e77a5d55](angular/angular@86e77a5) | feat | add Image directive skeleton ([#&#8203;45627](angular/angular#45627)) ([#&#8203;47082](angular/angular#47082)) |
| [0566205a02](angular/angular@0566205) | feat | Add image lazy loading and fetchpriority ([#&#8203;47082](angular/angular#47082)) |
| [4e952ba216](angular/angular@4e952ba) | feat | add loaders for cloudinary & imagekit ([#&#8203;47082](angular/angular#47082)) |
| [e854a8cdde](angular/angular@e854a8c) | feat | add loading attr to NgOptimizedImage ([#&#8203;47082](angular/angular#47082)) |
| [8d3701cb4c](angular/angular@8d3701c) | feat | add warnings re: image distortion ([#&#8203;47082](angular/angular#47082)) |
| [d5f7da2120](angular/angular@d5f7da2) | feat | define public API surface for NgOptimizedImage directive ([#&#8203;47082](angular/angular#47082)) |
| [d3c3426aa4](angular/angular@d3c3426) | feat | detect LCP images in `NgOptimizedImage` and assert if `priority` is set ([#&#8203;47082](angular/angular#47082)) |
| [451b85ca17](angular/angular@451b85c) | feat | explain why width/height is required ([#&#8203;47082](angular/angular#47082)) |
| [586274fe65](angular/angular@586274f) | feat | provide an ability to exclude origins from preconnect checks in NgOptimizedImage ([#&#8203;47082](angular/angular#47082)) |
| [57f3386e5b](angular/angular@57f3386) | feat | support custom srcset attributes in NgOptimizedImage ([#&#8203;47082](angular/angular#47082)) |
| [7baf9a46cd](angular/angular@7baf9a4) | feat | verify that priority images have preconnect links ([#&#8203;47082](angular/angular#47082)) |
| [f81765b333](angular/angular@f81765b) | feat | warn if rendered size is much smaller than intrinsic ([#&#8203;47082](angular/angular#47082)) |
| [e2ab99b95e](angular/angular@e2ab99b) | fix | allow null/undefined to be passed to ngClass input ([#&#8203;39280](angular/angular#39280)) ([#&#8203;46906](angular/angular#46906)) |
| [bedf537951](angular/angular@bedf537) | fix | allow null/undefined to be passed to ngStyle input ([#&#8203;47069](angular/angular#47069)) |
| [f9511bf6e8](angular/angular@f9511bf) | fix | avoid interacting with a destroyed injector ([#&#8203;47243](angular/angular#47243)) |
| [dc29e21b14](angular/angular@dc29e21) | fix | consider density descriptors with multiple digits as valid ([#&#8203;47230](angular/angular#47230)) |
| [801daf82d1](angular/angular@801daf8) | fix | detect `data:` and `blob:` inputs in `NgOptimizedImage` directive ([#&#8203;47082](angular/angular#47082)) |
| [fff8056e7f](angular/angular@fff8056) | fix | fix formatting on oversized image error ([#&#8203;47188](angular/angular#47188)) ([#&#8203;47232](angular/angular#47232)) |
| [1ca2ce19ab](angular/angular@1ca2ce1) | fix | remove default for image width ([#&#8203;47082](angular/angular#47082)) |
| [c5db867ddc](angular/angular@c5db867) | fix | remove duplicate deepForEach ([#&#8203;47189](angular/angular#47189)) |
| [1cf43deb18](angular/angular@1cf43de) | fix | sanitize `rawSrc` and `rawSrcset` values in NgOptimizedImage directive ([#&#8203;47082](angular/angular#47082)) |
| [d71dfe931f](angular/angular@d71dfe9) | fix | set bound width and height onto host element ([#&#8203;47082](angular/angular#47082)) |
| [32caa8b669](angular/angular@32caa8b) | fix | support density descriptors with 2+ decimals ([#&#8203;47197](angular/angular#47197)) ([#&#8203;47232](angular/angular#47232)) |
| [ae4405f0bf](angular/angular@ae4405f) | fix | throw if srcset is used with rawSrc ([#&#8203;47082](angular/angular#47082)) |
| [0c8eb8bc82](angular/angular@0c8eb8b) | perf | monitor LCP only for images without `priority` attribute ([#&#8203;47082](angular/angular#47082)) |

##### compiler-cli

| Commit | Type | Description |
| -- | -- | -- |
| [ea89677c12](angular/angular@ea89677) | feat | support more recent version of `tsickle` ([#&#8203;47018](angular/angular#47018)) |

##### core

| Commit | Type | Description |
| -- | -- | -- |
| [d1e83e1b30](angular/angular@d1e83e1) | feat | add `createComponent` function ([#&#8203;46685](angular/angular#46685)) |
| [10becab70e](angular/angular@10becab) | feat | add `reflectComponentType` function ([#&#8203;46685](angular/angular#46685)) |
| [4b377d3a6d](angular/angular@4b377d3) | feat | introduce createApplication API ([#&#8203;46475](angular/angular#46475)) |
| [31429eaccc](angular/angular@31429ea) | feat | support TypeScript 4.8 ([#&#8203;47038](angular/angular#47038)) |
| [796840209c](angular/angular@7968402) | fix | align TestBed interfaces and implementation ([#&#8203;46635](angular/angular#46635)) |

##### forms

| Commit | Type | Description |
| -- | -- | -- |
| [426af91a42](angular/angular@426af91) | feat | add `FormBuilder.record()` method ([#&#8203;46485](angular/angular#46485)) |
| [b302797de4](angular/angular@b302797) | fix | Correctly infer `FormBuilder` types involving `[value, validators]` shorthand in more cases. ([#&#8203;47034](angular/angular#47034)) |

##### language-service

| Commit | Type | Description |
| -- | -- | -- |
| [598b72bd05](angular/angular@598b72b) | feat | support fix the component missing member ([#&#8203;46764](angular/angular#46764)) |

##### platform-browser

| Commit | Type | Description |
| -- | -- | -- |
| [07606e3181](angular/angular@07606e3) | feat | add `isEmpty` method to the `TransferState` class ([#&#8203;46915](angular/angular#46915)) |

##### platform-server

| Commit | Type | Description |
| -- | -- | -- |
| [2b4d7f6733](angular/angular@2b4d7f6) | feat | support document reference in render functions ([#&#8203;47032](angular/angular#47032)) |

##### router

| Commit | Type | Description |
| -- | -- | -- |
| [0abb67af59](angular/angular@0abb67a) | feat | allow guards and resolvers to be plain functions ([#&#8203;46684](angular/angular#46684)) |
| [75df404467](angular/angular@75df404) | feat | Create APIs for using Router without RouterModule ([#&#8203;47010](angular/angular#47010)) |
| [10289f1f6e](angular/angular@10289f1) | feat | expose resolved route title ([#&#8203;46826](angular/angular#46826)) |
| [8600732b09](angular/angular@8600732) | feat | Expose the default matcher for `Routes` used by the `Router` ([#&#8203;46913](angular/angular#46913)) |
| [422323cee0](angular/angular@422323c) | feat | improve typings for RouterLink boolean inputs ([#&#8203;47101](angular/angular#47101)) |
| [26ea97688c](angular/angular@26ea976) | feat | Make router directives standalone ([#&#8203;46758](angular/angular#46758)) |
| [2a43beec15](angular/angular@2a43bee) | fix | Fix route recognition behavior with some versions of rxjs ([#&#8203;47098](angular/angular#47098)) |

##### service-worker

| Commit | Type | Description |
| -- | -- | -- |
| [383090858c](angular/angular@3830908) | feat | support `sendRequest` as a `notificationclick` action ([#&#8203;46912](angular/angular#46912)) |
| [3f548610dd](angular/angular@3f54861) | fix | export NoNewVersionDetectedEvent ([#&#8203;47044](angular/angular#47044)) |
| [482b6119c2](angular/angular@482b611) | fix | update golden `index.md` ([#&#8203;47044](angular/angular#47044)) |

#### Special Thanks

Alex Rickabaugh, Andrew Kushnir, Andrew Scott, Bob Watson, Cédric Exbrayat, Dylan Hunn, Emmanuel Roux, FatalMerlin, George Kalpakas, Ilia Mirkin, Jan Kuehle, Jeremy Elbourn, Jessica Janiuk, JiaLiPassion, Kalbarczyk, Kara Erickson, Katie Hempenius, Kristiyan Kostadinov, Merlin, Paul Gschwendtner, Pawel Kozlowski, Tristan Sprößer, Victor Porof, angular-robot\[bot], dario-piotrowicz, ivanwonder and vyom

<!-- CHANGELOG SPLIT MARKER -->

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xNzQuMiIsInVwZGF0ZWRJblZlciI6IjMyLjE3Ny4xIn0=-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1523
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants