Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

chore(*): update minimum Yarn version, do some Yarn-related cleanups #16714

Merged
merged 7 commits into from
Oct 11, 2018

Conversation

mgol
Copy link
Member

@mgol mgol commented Oct 11, 2018

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Build change.

What is the current behavior? (You can also link to an open issue here)

See the next section.

What is the new behavior (if this is a feature change)?

Included changes:

chore(*): update minimum Yarn version from 1.3.2 to 1.10.1

Yarn 1.10 added the integrity field to the lockfile, making newer Yarn users
have their lockfile changed a lot if they run yarn. This commit updates the
required Yarn version to be at least 1.10.1 and changes Travis & Jenkins to use
Yarn 1.10.1

chore(*): change the package.json's engines grunt field to grunt-cli

The grunt field suggested it's the grunt package version we're checking while
we check the grunt-cli version instead.

chore(*): stop separating Yarn script arguments from script names via " -- "

The " -- " separator is necessary in npm but not in Yarn. In fact, it's
deprecated in Yarn and some future version is supposed to start passing this
parameter directly to the scripts which may break them.

chore(*): Don't install grunt-cli globally during the build

It's enough to use yarn grunt instead of grunt and the global grunt-cli
installation is no longer needed.

Does this PR introduce a breaking change?

No.

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Fix/Feature: Docs have been added/updated
  • Fix/Feature: Tests have been added; existing tests pass

Other information:

mgol added 3 commits October 11, 2018 11:14
Yarn 1.10 added the integrity field to the lockfile, making newer Yarn users
have their lockfile changed a lot if they run `yarn`. This commit updates the
required Yarn version to be at least 1.10.1 and changes Travis & Jenkins to use
Yarn 1.10.1
The grunt field suggested it's the grunt package version we're checking while
we check the grunt-cli version instead.
… " -- "

The " -- " separator is necessary in npm but not in Yarn. In fact, it's
deprecated in Yarn and some future version is supposed to start passing this
parameter directly to the scripts which may break them.
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

🎉

package.json Outdated
@@ -10,8 +10,8 @@
},
"engines": {
"node": "^8.9.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change the node engine to be more lenient to newer versions of node too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure! Do we want to upgrade to Node 10 or stay at 8, just newer?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should make it relaxed enough to accept any engines that work with our build. So perhaps >= 8.9.1 < 11?

Copy link
Member Author

Choose a reason for hiding this comment

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

@petebacondarwin We don't have a way to verify if it keeps working at a version we don't work on. Also, .nvmrc needs to specify one version, we can't use 2 at the same time. So if we want to allow newer versions maybe it'd be better to say >=8.12.0 so that all newer versions are accepted?

Copy link
Contributor

Choose a reason for hiding this comment

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

Happy to go with >=8.12.0

@@ -21,21 +21,21 @@ rm -f angular.js.size


# BUILD #
yarn run grunt -- ci-checks package --no-color
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

Copy link
Member Author

Choose a reason for hiding this comment

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

That reminds me I can shorten it to yarn grunt instead of yarn run grunt... We already use the former form in a few places.

mgol added 4 commits October 11, 2018 13:58
It's enough to use `yarn grunt` instead of `grunt` and the global grunt-cli
installation is no longer needed.
As opposed to npm, `yarn binName` invokes a binary named `binName` exposed
by the respective package so the `grant` Yarn script is no longer needed.
@mgol mgol changed the title chore(*): Update minimum Yarn version, do some Yarn-related cleanups chore(*): update minimum Yarn version, do some Yarn-related cleanups Oct 11, 2018
@mgol mgol merged commit 0a5e58d into angular:master Oct 11, 2018
@mgol mgol deleted the yarn branch October 11, 2018 14:36
mgol added a commit that referenced this pull request Oct 11, 2018
Included changes:

*Update minimum Yarn version from 1.3.2 to 1.10.1*

Yarn 1.10 added the integrity field to the lockfile, making newer Yarn users
have their lockfile changed a lot if they run `yarn`. This commit updates the
required Yarn version to be at least 1.10.1 and changes Travis & Jenkins to use
Yarn 1.10.1

*Change the package.json's engines grunt field to grunt-cli*

The grunt field suggested it's the grunt package version we're checking while
we check the grunt-cli version instead.

*Stop separating Yarn script arguments from script names via " -- "*

The " -- " separator is necessary in npm but not in Yarn. In fact, it's
deprecated in Yarn and some future version is supposed to start passing this
parameter directly to the scripts which may break them.

*Don't install grunt-cli globally during the build*

It's enough to use `yarn grunt` instead of `grunt` and the global grunt-cli
installation is no longer needed.

*Use `yarn grunt` instead of `yarn run grunt`*

The former form is shorter.

*Don't define the `grunt` Yarn script*

As opposed to npm, `yarn binName` invokes a binary named `binName` exposed
by the respective package so the `grant` Yarn script is no longer needed.

*Allow Node versions newer than 8; bump the minimum*

Closes #16714
@Narretz
Copy link
Contributor

Narretz commented Oct 25, 2018

Just a note: when running yarn, I still get warning angularjs@: The engine "grunt-cli" appears to be invalid. I think we can just throw it out.

@mgol
Copy link
Member Author

mgol commented Oct 25, 2018

@Narretz we parse the engines fields ourselves for validation but Yarn does it as well. Those two are unrelated.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants