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

test: ensure correct TS version before running postinstall #29885

Closed
wants to merge 1 commit into from
Closed

test: ensure correct TS version before running postinstall #29885

wants to merge 1 commit into from

Conversation

filipesilva
Copy link
Contributor

@filipesilva filipesilva commented Apr 13, 2019 •
edited

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?

The bazel-schematics test could suffer from a version skew where new CLI projects were not yet using a new TS version, but Angular packages already were.

This caused the the ngc call in the added postinstall to run and fail: https://circleci.com/gh/angular/angular/283109

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@filipesilva filipesilva requested a review from a team as a code owner April 13, 2019 14:32
petebacondarwin
petebacondarwin reviewed Apr 13, 2019
View reviewed changes
integration/bazel-schematics/test.sh Outdated
yarn add typescript@3.4.2 --dev --ignore-scripts
yarn add typescript@3.4.2
# Run the postinstall script we renamed.
yarn renamed-postinstall
Copy link
Member

Choose a reason for hiding this comment

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

Also don't we want to hold off on the ngc until after installLocalPackages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think we do. I think this is actually why we were having inconsistent results... the initial install used the Angular NPM packages, for which TS was correct.

But those packages are not pinned and eventually the new Angular version was propagated to npm, which caused the version skew.

Good eye!

@petebacondarwin
Copy link
Member

This seems adequate:

  cd demo
  # Force more recent TS version until new Angular CLI projects also use it.
  yarn add typescript@3.4.2 --dev
  installLocalPackages

@filipesilva
test: ensure correct TS version before running postinstall
0316bdc 
The bazel-schematics test could suffer from a version skew where new CLI projects were not yet using a new TS version, but Angular packages already were.

This caused the the `ngc` call in the added `postinstall` to run and fail: https://circleci.com/gh/angular/angular/283109
@filipesilva
Copy link
Contributor Author

Yes, I agree. That way the postinstall runs several times but there are no script renaming shenenigans.

@filipesilva filipesilva added the target: major This PR is targeted for the next major release label Apr 13, 2019
@alxhub
Copy link
Member

alxhub commented Apr 13, 2019

Overriding the review requirements as this is breaking tests in master.

@alxhub alxhub added the action: merge The PR is ready for merge by the caretaker label Apr 13, 2019
@alxhub alxhub closed this in ea70d41 Apr 13, 2019
@filipesilva filipesilva deleted the integration-postinstall-ts branch April 13, 2019 17:47
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
@filipesilva @wKoza
test: ensure correct TS version before running postinstall (angular#2…
48c857b 
…9885)

The bazel-schematics test could suffer from a version skew where new CLI projects were not yet using a new TS version, but Angular packages already were.

This caused the the `ngc` call in the added `postinstall` to run and fail: https://circleci.com/gh/angular/angular/283109

PR Close angular#29885
@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 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@petebacondarwin petebacondarwin petebacondarwin approved these changes

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@filipesilva @petebacondarwin @alxhub @googlebot