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

build: upgrade yarn to 1.3.2 #21406

Closed
wants to merge 1 commit into from

Conversation

ocombe
Copy link
Contributor

@ocombe ocombe commented Jan 9, 2018

PR Type

What kind of change does this PR introduce?

[x] Build related changes

What is the current behavior?

We use yarn 1.0.2

Issue Number: #20566

What is the new behavior?

We use yarn 1.3.2

Does this PR introduce a breaking change?

[x] No

@ocombe ocombe requested a review from IgorMinar January 9, 2018 10:03
@ocombe ocombe added area: build & ci Related the build and CI infrastructure of the project action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release and removed cla: yes labels Jan 9, 2018
@mary-poppins
Copy link

You can preview 2d74c85 at https://pr21406-2d74c85.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 06425b3 at https://pr21406-06425b3.ngbuilds.io/.

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.

don't we need to update the docker container? // @alexeagle

if we merge this as is, circle is going to use wrong version of yarn.

I'm also shocked to see that there are no changes to the yarn.lock file. Is this expected? if updated circle docker container is also green then we should merge this as is without any lock file changes.

thanks!

@alexeagle
Copy link
Contributor

@IgorMinar should be good:

$ docker run --rm -it angular/ngcontainer:0.0.8
circleci@52269916ee5f:/$ yarn --version
1.3.2

@IgorMinar IgorMinar added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 10, 2018
@ocombe
Copy link
Contributor Author

ocombe commented Jan 10, 2018

Yes, the circle ci docker is already using 1.3.2 apparently.
The lockfile is compatible with 1.3.2, nothing will change until we add new dependencies

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.

Awesome then!

@IgorMinar IgorMinar added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jan 10, 2018
@IgorMinar
Copy link
Contributor

Once this lands in master and someone will use an old yarn version they'll get this error:

error angular.io@0.0.0: The engine "yarn" is incompatible with this module. Expected version ">=1.0.2 <1.2.0".

I think the error is actionable enough that we don't need a special team wide announcement. Let's get this in! :)

@IgorMinar
Copy link
Contributor

@ocombe could you add a change to this PR (or send a new one) that would delegate the yarn vs npm check into a file in order to hide the ugly line "$ node -e "if(process.env.npm_execpath.indexOf('yarn') " that appears every time you run a yarn command. This line looks like an error, but it's really just a command that contains a possible error message, but that's not easy to tell for someone who's not familiar with the setup.

Current output for yarn install:

$ yarn install
yarn install v1.3.2
$ node -e "if(process.env.npm_execpath.indexOf('yarn') === -1) throw new Error('Please use Yarn instead of NPM to install dependencies. See: https://yarnpkg.com/lang/en/docs/install/')"
[1/5] 🔍  Validating package.json...
[2/5] 🔍  Resolving packages...
[3/5] 🚚  Fetching packages...
...

Desired output for yarn install:

yarn install v1.3.2
$ tools/yarn/yarn-check.js
[1/5] 🔍  Validating package.json...
[2/5] 🔍  Resolving packages...
[3/5] 🚚  Fetching packages...
...

@ocombe
Copy link
Contributor Author

ocombe commented Jan 10, 2018

Sure I'll do that right now

@IgorMinar
Copy link
Contributor

thanks!

@ocombe ocombe force-pushed the feat/#20566-update-yarn branch 3 times, most recently from 2c18701 to c399cdc Compare January 10, 2018 09:20
@angular angular deleted a comment from mary-poppins Jan 10, 2018
@angular angular deleted a comment from mary-poppins Jan 10, 2018
@mary-poppins
Copy link

You can preview cff027c at https://pr21406-cff027c.ngbuilds.io/.

@angular angular deleted a comment from mary-poppins Jan 10, 2018
alexeagle pushed a commit that referenced this pull request Jan 10, 2018
@alexeagle alexeagle closed this in 7493b8a Jan 10, 2018
@ocombe ocombe deleted the feat/#20566-update-yarn branch January 11, 2018 16:45
leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
@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 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: build & ci Related the build and CI infrastructure of the project cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants