Skip to content

build: TypeScript 3.6 compatibility. - #32908

Closed
mprobst wants to merge 1 commit into
angular:masterfrom
mprobst:ts36-compat
Closed

build: TypeScript 3.6 compatibility.#32908
mprobst wants to merge 1 commit into
angular:masterfrom
mprobst:ts36-compat

Conversation

@mprobst

@mprobst mprobst commented Sep 30, 2019

Copy link
Copy Markdown
Contributor

This PR updates Angular to compile with TypeScript 3.6 while retaining
compatibility with TS3.5. We achieve this by inserting several as any
casts for compatiblity around ts.CompilerHost APIs.

@mprobst mprobst added area: build & ci Related the build and CI infrastructure of the project refactoring Issue that involves refactoring or code-cleanup labels Sep 30, 2019
@mprobst
mprobst requested review from a team and kara September 30, 2019 07:42
@ngbot ngbot Bot added this to the needsTriage milestone Sep 30, 2019
@mprobst

mprobst commented Sep 30, 2019

Copy link
Copy Markdown
Contributor Author

@kara the area really is all, but that doesn't appear to be a category.

LMK if this works for you with the anys. I believe it'd also be possible to introduce some indirection of the TS types for the backwards compat, but that'd require cross-component dependencies and IMHO wouldn't really buy us much, compared to the few call sites.

petebacondarwin
petebacondarwin suggested changes Sep 30, 2019
View reviewed changes
Comment thread packages/compiler-cli/ngcc/src/host/commonjs_host.ts Resolved
Comment thread packages/compiler-cli/ngcc/src/host/umd_host.ts Resolved
Comment thread packages/compiler-cli/src/metadata/collector.ts Resolved
Comment thread packages/compiler-cli/src/ngtsc/util/src/typescript.ts Resolved
Comment thread packages/core/src/application_ref.ts Resolved
Comment thread packages/core/src/render3/interfaces/definition.ts Outdated

@petebacondarwin petebacondarwin Sep 30, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why remove the const?

@mprobst mprobst Sep 30, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Const enums are guaranteed to be fully removed and inlined to their members. But we re-export the enum, which means the object would need to be retained. Here's the error:

 error TS2475: 'const' enums can only be used in property or index access expressions or the right hand side of an import declaration or export assignment or type query.

@gkalpak gkalpak Sep 30, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder if this is used at all. I couldn't find any reference to DirectiveDefFlags (other than declaring it and exporting it) and it doesn't seem to be part of the public API either.

EDIT: It seems to be an artifact from #20855.

@IgorMinar IgorMinar Oct 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I dropped this in #32946

gkalpak
gkalpak approved these changes Sep 30, 2019
View reviewed changes

@gkalpak gkalpak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM (except for the commit messages, which need to follow the guidelines to make the linter happy) ❤️

@mprobst

mprobst commented Sep 30, 2019

Copy link
Copy Markdown
Contributor Author

@gkalpak do you mean the commit msg from the subsequent fixups, or the original one? I hope the original one is OK, let me know if it's not.

I've rebased onto master and squashed.

@gkalpak

gkalpak commented Sep 30, 2019

Copy link
Copy Markdown
Member

I meant the subsequent commits (which are gone now). But now that you mentioned it, the original commit should probably not mention compiler-cli as the scope, since changes are made to other packages as well 😁

Also, the formatter complains about misformatted files now. (You can see the error here.)
You can fix it with gulp format.

BTW, it is outside the scope of this PR, but mentioning it "for completeness":
There is some more work needed to make Angular officially support TS3.6. (See, for example, the equivalent PR for adding support for TS3.5: #31615)

@mprobst mprobst changed the title build(compiler-cli): TypeScript 3.6 compatibility. build: TypeScript 3.6 compatibility. Sep 30, 2019
@mprobst

mprobst commented Sep 30, 2019

Copy link
Copy Markdown
Contributor Author

@gkalpak done.

@gkalpak gkalpak mentioned this pull request Oct 2, 2019
Closed
2 of 3 tasks
IgorMinar
IgorMinar approved these changes Oct 2, 2019
View reviewed changes

@IgorMinar IgorMinar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm, I created a follow up PR that properly upgrades to tsc 3.6 and cleans up some of Martin's changes: #32946 this PR is blocked on typscript 3.6.4 release, so I think we should go ahead and merge this one in to unblock tsc update in google3, while we wait on the typescript release.

@IgorMinar IgorMinar 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 action: presubmit The PR is in need of a google3 presubmit labels Oct 2, 2019
@mprobst
build: TypeScript 3.6 compatibility.
3a7e154 
This PR updates Angular to compile with TypeScript 3.6 while retaining
compatibility with TS3.5. We achieve this by inserting several `as any`
casts for compatiblity around `ts.CompilerHost` APIs.
@mprobst

mprobst commented Oct 2, 2019

Copy link
Copy Markdown
Contributor Author

@IgorMinar I've rebased this onto latest master.

@IgorMinar

IgorMinar commented Oct 2, 2019

Copy link
Copy Markdown
Contributor

merge-assistance: global approval

I also kicked off a presubmit: http://test/OCL:272419562:BASE:272419580:1570020811421:232c8db4

@IgorMinar IgorMinar added the target: major This PR is targeted for the next major release label Oct 3, 2019
@atscott atscott closed this in 5332b04 Oct 3, 2019
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Nov 3, 2019

Copy link
Copy Markdown

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 Nov 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@kara kara Awaiting requested review from kara
4 more reviewers
@petebacondarwin petebacondarwin petebacondarwin approved these changes
@IgorMinar IgorMinar IgorMinar approved these changes
@JiaLiPassion JiaLiPassion JiaLiPassion approved these changes
@gkalpak gkalpak gkalpak approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit area: build & ci Related the build and CI infrastructure of the project cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note refactoring Issue that involves refactoring or code-cleanup target: major This PR is targeted for the next major release

Projects

None yet

Milestone

needsTriage

Development

Successfully merging this pull request may close these issues.

6 participants

@mprobst @gkalpak @IgorMinar @petebacondarwin @JiaLiPassion @googlebot