Skip to content

build: ensure build uses typescript 3.9 #19336

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
Merged
merged 1 commit into from
May 18, 2020
Merged

build: ensure build uses typescript 3.9 #19336

merged 1 commit into from
May 18, 2020

Conversation

devversion
Copy link
Member

@devversion devversion commented May 12, 2020 •
edited
Loading

We recently updated TS 3.9 in the package.json with #19286, but the build did
not pick up the new version. This is because peer dependency ranges
did not accept TS 3.9, so yarn hoisted satisfying TS versions.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 12, 2020
@devversion devversion force-pushed the build/treally-update-to-ts-3.9 branch 7 times, most recently from b60623a to 0433554 Compare May 13, 2020 12:22
devversion
devversion commented May 13, 2020
View reviewed changes
src/bazel-tsconfig-test.json
@@ -4,7 +4,7 @@
{
"extends": "./bazel-tsconfig-build.json",
"compilerOptions": {
"importHelpers": false,
"importHelpers": true,
Copy link
Member Author

@devversion devversion May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This needs to be set to true because the inline emitted helpers by TypeScript are currently broken in TS 3.9.2. A fix has landed, but will be available in the next patch..

microsoft/TypeScript#38501. We can leave this enabled anyway, or turn it back to false. Both have pros and cons.

Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW: The CLI has importHelpers: true for tests too.

src/cdk/schematics/BUILD.bazel
module_name = "@angular/cdk/schematics",
prodmode_module = "commonjs",
Copy link
Member Author

Choose a reason for hiding this comment

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

The schematic code does not rely on importHelpers / tslib, so it currently breaks due to the aforementioned TS bug. We can fix this by building schematics always in commonjs. This might generally make sense to keep since schematics do not run in the browser.

@devversion devversion added the target: major This PR is targeted for the next major release label May 13, 2020
@devversion devversion marked this pull request as ready for review May 13, 2020 12:54
@devversion devversion requested review from crisbeto, jelbourn, mmalerba and a team as code owners May 13, 2020 12:54
@devversion devversion added this to the 10.0.0 milestone May 13, 2020
@devversion devversion added the P2 The issue is important to a large percentage of users, with a workaround label May 13, 2020
@jelbourn
Copy link
Member

Is this also blocked on angular/angular#36989?

@devversion
Copy link
Member Author

devversion commented May 13, 2020 •
edited
Loading

@jelbourn Not necessarily as we:

  1. Ignored the @angular/compiler-cli MAX_TS version using the disableVersionCheck option.
  2. Overwrote the peer dependency range from @angular/compiler-cli on TypeScript through Yarn resolutions
  3. Are not affected by the ngcc issues that surfaced with TS 3.9 because we use tsickle for decorator downleveling (which ngcc properly handles)

I guess it's up to preference whether we want to merge this or not. We can certainly wait until framework lands their PR so that we don't need (1) an (2), but we can also land this and remove (1) and (2) in a follow-up.

@devversion devversion added the blocked This issue is blocked by some external factor, such as a prerequisite PR label May 13, 2020
@alan-agius4 alan-agius4 mentioned this pull request May 15, 2020
Closed
kara pushed a commit to angular/angular that referenced this pull request May 18, 2020
@alan-agius4 @kara
ci: temporary disable components-repo-unit-tests (#37129)
844208f 
'components-repo-unit-tests'  CI job has been temporary disabled until the Components team support building and testing their repo with TypeScript 3.9. The TS 3.9 update is being done in angular/components#19336. Once this gets merged we should re-enable this CI job.

More context on why this had to be disabled can be found: #37129 (comment)

PR Close #37129
@devversion devversion force-pushed the build/treally-update-to-ts-3.9 branch from 0433554 to f96e178 Compare May 18, 2020 16:47
@devversion
build: ensure build uses typescript 3.9
772d6e9 
We recently updated TS 3.9 in the `package.json` with
angular#19286, but
the build did not pick up the new version. This is
because peer dependency ranges did not accept TS 3.9,
so yarn hoisted sufficient TS versions.
@devversion devversion force-pushed the build/treally-update-to-ts-3.9 branch from f96e178 to 772d6e9 Compare May 18, 2020 16:55
@devversion devversion removed the blocked This issue is blocked by some external factor, such as a prerequisite PR label May 18, 2020
jelbourn
jelbourn approved these changes May 18, 2020
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added lgtm action: merge The PR is ready for merge by the caretaker labels May 18, 2020
@mmalerba mmalerba merged commit a86064a into angular:master May 18, 2020
devversion added a commit to devversion/angular that referenced this pull request May 18, 2020
@devversion
ci: update components repo unit tests job commit
b8e32a5 
Updates the commit the `components-repo-unit-tests` job runs
against. The goal is that we run against a revision that at
least contains: angular/components#19336.

The new commit contains fixes for a flaky test in the datepicker that we
saw failing in the components-repo-unit-tests job too:
https://app.circleci.com/pipelines/github/angular/angular/15359/workflows/27ffae7c-a7b8-46a3-9c9e-6dd22ca4734d/jobs/712643.

Additionally, with this commit, the components repo unit tests job will
use TypeScript 3.9.2, so we can re-enable the job in another commit.
kara pushed a commit to angular/angular that referenced this pull request May 18, 2020
@devversion @kara
ci: update components repo unit tests job commit (#37176)
afd58b3 
Updates the commit the `components-repo-unit-tests` job runs
against. The goal is that we run against a revision that at
least contains: angular/components#19336.

The new commit contains fixes for a flaky test in the datepicker that we
saw failing in the components-repo-unit-tests job too:
https://app.circleci.com/pipelines/github/angular/angular/15359/workflows/27ffae7c-a7b8-46a3-9c9e-6dd22ca4734d/jobs/712643.

Additionally, with this commit, the components repo unit tests job will
use TypeScript 3.9.2, so we can re-enable the job in another commit.

PR Close #37176
@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 Jun 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn approved these changes

@crisbeto crisbeto Awaiting requested review from crisbeto

@mmalerba mmalerba Awaiting requested review from mmalerba

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P2 The issue is important to a large percentage of users, with a workaround target: major This PR is targeted for the next major release
Projects
None yet
Milestone
10.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@devversion @jelbourn @googlebot @mmalerba