-
Notifications
You must be signed in to change notification settings - Fork 27.5k
chore(*): update Node.js from 8 to 12, update some dependencies #16915
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (as long as Travis is happy 😱)
27017bb
to
eea94b1
Compare
|
You can support Gulp Upgrading to Gulp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The primordials failures with Gulp v3
are caused by NodeJS 12+ no longer accepting the natives
override. The PR I linked above has another approach that solves these issues with graceful-fs
.
That's a great idea, thanks! I'll have a look later. |
Adding a resolution for graceful-fs works at treat. |
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
1 similar comment
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
@googlebot I consent |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
1 similar comment
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Looks like there's a duplicate Googlebot here. We had this problem in AngularJS Material too. I think that there was one configured for the repo and one for the org. We were able to resolve it. I don't know if this affects only this PR or all PRs in the repo. @josephperrott maybe dev-infra can resolve easily? |
495a877
to
bfe4b99
Compare
Node.js 8 ends its support at the end of 2019. Node 12 will be supported until April 2022 which is way past AngularJS end of support. Some dependencies needed to be updated to make them work in Node.js 12.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just a note that Chrome M83 is the latest stable now and NodeJS 12.16.3 is the latest LTS.
capabilitiesForSauceLabs({ | ||
browserName: 'chrome', | ||
platform: 'OS X 10.14', | ||
version: '81' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chrome Stable just hit 83
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to this https://saucelabs.com/platform/supported-browsers-devices SauceLabs only has Chrome 81 available right now.
This commit gets rid of all references to Travis and, belatedly, Jenkins. Now all CI is done on CircleCI and releases are run locally. The CI no longer updates the docs and code.angularjs.org for jobs that are not on the `master` branch. During releases, the docs and code should be uploaded manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Post-merge review 😁
Mostly some questions.
@@ -98,11 +99,17 @@ | |||
"sorted-object": "^1.0.0", | |||
"stringmap": "^0.2.2" | |||
}, | |||
"dependencies": {}, | |||
"dependencies": { | |||
"firebase": "^7.14.4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need firebase
as a dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. I think that got included when I was messing about with trying out firebase deployments...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
firebase-tools
makes sense as a dev dependency 😄
"dependencies": { | ||
"firebase": "^7.14.4", | ||
"firebase-tools": "^8.3.0", | ||
"node-glob": "^1.2.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need these three to be dependencies (vs devDependencies)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly - I added this by accident.
@@ -36,7 +36,6 @@ function init { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this file be executable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely! I forgot to re-apply the chmod
.
@@ -38,4 +38,4 @@ function run { | |||
phase publish |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this file be executable?
@@ -123,6 +123,8 @@ describe('$anchorScroll', function() { | |||
var lastAnchor = anchors.last(); | |||
var lastAnchorId = 'anchor-5'; | |||
|
|||
if (browser.params.browser === 'firefox') return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this? Are the tests not working on Firefox? Or is $anchorScroll()
not working correctly on Firefox?
It would be helpful to have a comment explaining at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
steps: | ||
- custom_attach_workspace | ||
- init_environment | ||
- run: yarn -s grunt ci-checks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the -s
here (but not elsewhere)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied it from the angular/angular project where it is used extensively. Then I forgot about it. I'll remove it because I don't think it makes much difference and is confusing if not used consistently.
filters: | ||
branches: | ||
only: | ||
- latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is latest
a special value? Are there any docs on how it works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously deployments of the "latest" build, i.e. the releases that have a value of latest
in the dist-tag
property of package.json also triggered deployment of the docs to a firebase hosting, and also a special version of the code to snapshot-stable
.
This is more difficult in CircleCI, so I had this idea that we would simply keep a new latest
git branch updated to the latest stable release, which would then trigger the deployments.
I never got it fleshed out properly - and now I am thinking, given the status of the project, that we can just run manual deployments when we run the release. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it more difficult in CircleCI. It seems quite straight forward to me (just check package.json > distTag
and abort the job if it is not latest
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I guess that could work. I was thinking of ways to avoid running the job in the first place.
# the commit is tagged | ||
# - or the branch is "master" | ||
# - or the branch distTag from package.json is "latest" (i.e. stable branch) | ||
if [[ "$TRAVIS_TAG" != '' || "$TRAVIS_BRANCH" == master || "$DIST_TAG" == latest ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has this logic been replicated in the new setup? I only see the corresponding job be run on branches master and latest (whatever that means 😁). What about tagged commits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic has not been mapped over 1 to 1. See #16915 (comment)
A tagged commit corresponds to a release, right? So if our release process includes deployment of docs and code, then we are done. The only missing piece is the dist-tag updates...
fi | ||
|
||
if [[ "$DEPLOY_DOCS" == true || "$DEPLOY_CODE" == true ]]; then | ||
grunt prepareDeploy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that grunt prepareDeploy
is run for both DEPLOY_CODE
and DEPLOY_DOCS
here. In the new setup, it is only run in the deploy-docs
job (but not the deploy-code
one. Is that intended?
This is a follow-up to angular#16915, cleaning up `.circleci/config.yml` (pinning Docker images of executors, removing unused code, etc.).
This is a follow-up to angular#16915, cleaning up `package.json` and `.circleci/config.yml` and making release scripts executable.
This is a follow-up to angular#16915, cleaning up `package.json` and `.circleci/config.yml` and making release scripts executable.
This is a follow-up to angular#16915, cleaning up `package.json` and `.circleci/config.yml` and making release scripts executable.
This is a follow-up to angular#16915, cleaning up `package.json` and `.circleci/config.yml` and making release scripts executable.
This is a follow-up to #16915, cleaning up `package.json` and `.circleci/config.yml` and making release scripts executable.
|
AngularJS is in LTS mode
We are no longer accepting changes that are not critical bug fixes into this project.
See https://blog.angular.io/stable-angularjs-and-long-term-support-7e077635ee9c for more detail.
Does this PR fix a regression since 1.7.0, a security flaw, or a problem caused by a new browser version?
What is the current behavior? (You can also link to an open issue here)
Node.js 8 is used to build/test AngularJS.
What is the new behavior (if this is a feature change)?
Node.js 12 is used to build/test AngularJS.
Does this PR introduce a breaking change?
No.
Please check if the PR fulfills these requirements
Fix/Feature: Docs have been added/updatedFix/Feature: Tests have been added; existing tests passOther information:
Node.js 8 ends its support at the end of 2019. Node 12 will be supported
until April 2022 which is way past AngularJS end of support.
Some dependencies needed to be updated to make them work in Node.js 12.
TODO: