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): add TS migration #33250

Closed
wants to merge 1 commit into from
Closed

feat(core): add TS migration #33250

wants to merge 1 commit into from

Conversation

filipesilva
Copy link
Contributor

@filipesilva filipesilva commented Oct 18, 2019

Blocked on #32946

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@filipesilva filipesilva added state: blocked target: major This PR is targeted for the next major release labels Oct 18, 2019
@mary-poppins
Copy link

You can preview 271ee0f at https://pr33250-271ee0f.ngbuilds.io/.

@filipesilva filipesilva marked this pull request as ready for review October 18, 2019 17:21
@filipesilva filipesilva requested a review from a team as a code owner October 18, 2019 17:21
@filipesilva filipesilva requested review from kara and removed request for devversion October 18, 2019 17:21
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM schematic code-wise. What's the plan for these type of migrations in the future?

On the angular/components side we have this problem that we need to keep migrations forever in our release package due to the way ng update works (i.e. it installs the latest version and runs migrations for older versions). We are planning on talking about this at some point with the tooling team since we do not want to store all migration code for old versions in our release output (it will blow-up over time).

Same would apply here. I'm just trying to raise awareness as I think that it's not ideal to keep migrations forever in the code-base, plus not all developers use the CLI and we will unnecessarily bloat the @angular/core package for them (worst case slowing down install if we add more over time). Weren't these type of migrations part of the CLI in the past?

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.

This migration has two distinct parts:

  • typescript update
  • @types/node update

the first one should not be necessary because @angular/core depends on the new version of typescript and ng-update will update the project's typescript version to match that.

the second update of @types/node is not relevant for @angular/core. That dependency is introduced into CLI projects by the CLI scaffolding so that we can write e2e tests in typescript - these tests run in node, hence the need for these typings.

I propose that we remove the migration for the typescript update and move the migration for @types/node into CLI rather than @angular/core.

@filipesilva
Copy link
Contributor Author

Moving to CLI.

@devversion I am not aware of what that part of the ng update design is. @clydin might know.

filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Oct 18, 2019
Angular will only support TS 3.6+ on version 9, and older versions of `@types/node` are incompatible with it and will cause all builds to fail.

Related to angular/angular#33250
alan-agius4 pushed a commit to angular/angular-cli that referenced this pull request Oct 19, 2019
Angular will only support TS 3.6+ on version 9, and older versions of `@types/node` are incompatible with it and will cause all builds to fail.

Related to angular/angular#33250
vikerman pushed a commit to angular/angular-cli that referenced this pull request Oct 21, 2019
Angular will only support TS 3.6+ on version 9, and older versions of `@types/node` are incompatible with it and will cause all builds to fail.

Related to angular/angular#33250
@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 Nov 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants