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.

Sign up for GitHub

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

Jump to bottom

Support TypeScript 4.1 #39571

Closed
wants to merge 9 commits into from
Closed

Support TypeScript 4.1 #39571

wants to merge 9 commits into from

Conversation

clydin
Copy link
Member

@clydin clydin commented Nov 5, 2020 •
edited

This change updates the framework to support TypeScript 4.1.

@google-cla google-cla bot added the cla: yes label Nov 5, 2020
@clydin clydin force-pushed the ts-4.1 branch 2 times, most recently from 2b39f9f to a40cfd7 Compare November 5, 2020 15:15
@google-cla

This comment has been minimized.

Sign in to view
@google-cla google-cla bot added cla: no and removed cla: yes labels Nov 5, 2020
@JoostK

This comment has been minimized.

Sign in to view
@google-cla google-cla bot added cla: yes and removed cla: no labels Nov 5, 2020
@josephperrott josephperrott added the area: build & ci Related the build and CI infrastructure of the project label Nov 6, 2020
@ngbot ngbot bot added this to the needsTriage milestone Nov 6, 2020
JoostK
JoostK reviewed Nov 6, 2020
View reviewed changes
packages/compiler-cli/ngcc/src/host/commonjs_umd_utils.ts
@@ -78,7 +78,7 @@ export function findNamespaceOfIdentifier(id: ts.Identifier): ts.Identifier|null
export function findRequireCallReference(id: ts.Identifier, checker: ts.TypeChecker): RequireCall|
null {
const symbol = checker.getSymbolAtLocation(id) || null;
const declaration = symbol && symbol.valueDeclaration;
const declaration = symbol?.valueDeclaration ?? symbol?.declarations?.[0];
Copy link
Member

Choose a reason for hiding this comment

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

This change seems to have been due to microsoft/TypeScript#39770, where the variable declaration is now treated as an alias declaration which no longer has a value declaration.

alan-agius4
alan-agius4 reviewed Nov 9, 2020
View reviewed changes
Copy link
Contributor

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

Can we add an integration tests like https://github.com/angular/angular/tree/master/integration/typings_test_ts40

@alan-agius4 alan-agius4 mentioned this pull request Nov 20, 2020
Closed
@clydin clydin added target: minor This PR is targeted for the next minor release and removed state: blocked labels Nov 20, 2020
@clydin clydin mentioned this pull request Nov 20, 2020
Merged
@clydin clydin marked this pull request as ready for review November 20, 2020 20:49
gkalpak
gkalpak approved these changes Nov 20, 2020
View reviewed changes
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

LGTM for dev-infra, docs-infra, fw-upgrade.

From a quick look, I would expect the paths without baseUrl breaking change to affect ngcc, because we iirc we assume baseUrl will be present. For example, ngcc seems to ignore paths if there is no baseUrl.

Reviewed-for: dev-infra, docs-infra, fw-upgrade

aio/package.json Outdated Show resolved Hide resolved
integration/typings_test_ts41/include-all.ts Show resolved Hide resolved
packages/compiler-cli/test/transformers/program_spec.ts Show resolved Hide resolved
clydin and others added 4 commits November 20, 2020 17:14
@clydin
feat(compiler-cli): add support for using TypeScript 4.1
e9d1ded 
This change enables projects to be built with TypeScript 4.1.  Support for TypeScript 4.0 is also retained.
@clydin
build: support building with TypeScript 4.1
310a054 
TypeScript 4.1 is now used to build and test within the repository.
@clydin
fixup! build: support building with TypeScript 4.1
a0cb33c
@JoostK @clydin
fixup! build: support building with TypeScript 4.1
6de5ef0
@jpike88
Copy link

jpike88 commented Nov 23, 2020

Just checking... I got the error Internal Error: The structure of the program changed during codegen when attempting to use typescript 4.1.2 with angular cli 11.0.2. Is this expected, and covered in the PR's fixes? I can't seem to find anyone mentioning that kind of error, and not sure what to make of it.

@JoostK
Copy link
Member

JoostK commented Nov 23, 2020

Just checking... I got the error Internal Error: The structure of the program changed during codegen when attempting to use typescript 4.1.2 with angular cli 11.0.2. Is this expected, and covered in the PR's fixes? I can't seem to find anyone mentioning that kind of error, and not sure what to make of it.

Yes that is expected for ViewEngine compilations, but not Ivy. And yes that is covered in this PR.

JiaLiPassion
JiaLiPassion approved these changes Nov 23, 2020
View reviewed changes
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
Reviewed-for: zone-js

IgorMinar
IgorMinar approved these changes Nov 25, 2020
View reviewed changes
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

Reviewed-for: global-approvers

@IgorMinar IgorMinar added the action: merge The PR is ready for merge by the caretaker label Nov 25, 2020
jessicajaniuk pushed a commit that referenced this pull request Nov 25, 2020
@clydin @thePunderWoman
build: support building with TypeScript 4.1 (#39571)
318255a 
TypeScript 4.1 is now used to build and test within the repository.

PR Close #39571
jessicajaniuk pushed a commit that referenced this pull request Nov 25, 2020
@clydin @thePunderWoman
test(compiler-cli): add TypeScript 4.1 typings integration test (#39571)
4def19f 
This change adds a typings test identical to the TypeScript 4.0 test but using TypeScript 4.1 instead.

PR Close #39571
@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 Dec 26, 2020
@pullapprove pullapprove bot removed the area: build & ci Related the build and CI infrastructure of the project label Dec 26, 2020
@ngbot ngbot bot removed this from the needsTriage milestone Dec 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JoostK JoostK JoostK left review comments

@gkalpak gkalpak gkalpak approved these changes

@alan-agius4 alan-agius4 alan-agius4 left review comments

@IgorMinar IgorMinar IgorMinar approved these changes

@JiaLiPassion JiaLiPassion JiaLiPassion approved these changes

@alxhub alxhub alxhub approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@clydin @JoostK @jpike88 @IgorMinar @JiaLiPassion @alxhub @gkalpak @alan-agius4 @josephperrott