Build: update Node to version 8 LTS (carbon) #19317

sirreal · 2017-10-31T10:00:18Z

This PR updates Calypso to node v8.9.1 which is the latest LTS release. 🎉

This PR closely follows #16820 to upgrade node to version 8.9.1.

Testing

Local Dev
- Switch to node version 8.9.1 (npm@5.1.1)
```
nvm install 8 && nvm use 8
```
- npm run distclean
- npm start
- No functional changes to Calypso
Local Docker Image
- Follow image instructions at: PCYsg-5XR-p2 or
- Setup docker locally and add the following host entry:
```
127.0.0.1 wpcalypso.wordpress.com
```
- npm run build-docker
- npm run docker
Desktop
- Clone the wp-desktop project
- Follow install instructions
- In the calypso submodule checkout this branch
- Desktop behaves normally
Calypso as a dependency
- @DanReyLop may have better information about who this may be.

Blockers

official LTS release
Need to publish a new release of https://github.com/Automattic/photon.js
Use new photon version Deps: Update to photon@2.0.1 #19632
calypso-prettier sha dependency issue ( Upgrade Prettier to version 1.7.4 #19519)

matticbot · 2017-10-31T10:00:19Z

Test live: https://calypso.live/?branch=update/node-8-8-1

sirreal · 2017-10-31T10:06:04Z

@DanReyLop Do you have familiarity with package-lock.json and how we should modify our processes? It doesn't appear to be generated by our update-deps script.

sirreal · 2017-10-31T10:07:24Z

Labeled as blocked awaiting official LTS announcement…

DanReyLop · 2017-10-31T10:51:30Z

Do you have familiarity with package-lock.json and how we should modify our processes? It doesn't appear to be generated by our update-deps script.

Not familiar, but I'll never stop learning :)

I vaguely recall that we want to have both package-lock and shrinkwrap, they are mostly the same but they're used in different circumstances (installing wp-calypso vs fetching wp-calypso as a dependency). But I'll need to look at this again.

Give me the rest of today, I'll look at this in the afternoon.

sirreal · 2017-10-31T11:05:16Z

npm install is failing on this branch.

6719 error Command failed: /usr/local/bin/git checkout 801d65585af6995a223136862944095b48f5c567
6719 error fatal: reference is not a tree: 801d65585af6995a223136862944095b48f5c567

This looks like an indirect dependency installing by sha. I'm not sure where this is happening.

sirreal · 2017-10-31T11:22:46Z

This looks like an indirect dependency installing by sha. I'm not sure where this is happening.

O HAI

https://github.com/Automattic/calypso-prettier/blob/14307ba5cc6bcfc00243f5e2d1b25c72d52c28c8/package.json#L39

"typescript-eslint-parser": "git://github.com/eslint/typescript-eslint-parser.git#801d65585af6995a223136862944095b48f5c567"

Bumping calypso-prettier to 1.7 fixes this problem. @jsnajdr Anything holding us back from updating to 1.7 at this point?

jsnajdr · 2017-10-31T11:28:10Z

Anything holding us back from updating to 1.7 at this point?

Nothing at all -- the Calypso fork has an up-to-date branch with 1.7.1 I created a few weeks ago. Give me ~24 hours to update to the latest 1.7.4 and create an upgrade PR.

gziolo · 2017-10-31T11:50:03Z

Do you have familiarity with package-lock.json and how we should modify our processes? It doesn't appear to be generated by our update-deps script.

Isn't npm 5 able to reuse existing shrinkwrap file and use it instead of package-lock.json? As far as I understood it listening to podcasts, the idea is to have shrinkwrap to support both npm 5 and earlier versions of npm. Not sure how that works in practice, but it seems like a good approach.

sirreal · 2017-10-31T11:54:56Z

Isn't npm 5 able to reuse existing shrinkwrap file and use it instead of package-lock.json?

@gziolo I also get the impression that shrinkwrap has precedence over lock and we may not need to make changes around package-lock:

To prevent this potential issue, npm uses package-lock.json or, if present, npm-shrinkwrap.json. These files are called package locks, or lockfiles.

☝️ from npm-package-locks

simison · 2017-10-31T11:58:29Z

they are mostly the same but they're used in different circumstances (installing wp-calypso vs fetching wp-calypso as a dependency).

Yep!

My thumb of rule with these files has been:

package-lock is for projects (those with private: true at package.json) and should always be committed. package-lock cannot be published to NPM.
shrinkwrap is for CLI modules (like calypso-prettier?) and sometimes for required modules (like i18n-calypso), but that's generally discouraged to give project developers more freedom...

If you end up having both, package-lock is ignored.

Considerations:

With this many devs poking in the codebase, shrinkwrap might still be easier to handle because it requires manual npm shrinkwrap before that file gets updated (I think? I haven't really used it for a while anymore). package-lock gets updated on each npm install/update.
shrinkwrap works also with older NPM versions, if that matters...

My take:
→ Calypso should maybe have only package-lock?
→ Since they indeed are pretty much identical, just replacing everything shrinkwrap'ish with package-lock should work!

Hope this helps to decide!

simison · 2017-10-31T12:03:11Z

BTW this was lovely, on updating Node v6 to 8:

That’s a pretty decent 23% reduction in render time. Or, more concretely, a 23% reduction in the hardware required to serve the site.
...
I’m going to suggest to my employer that we upgrade to Node 8, reduce our Amazon instances from 25 to 20 and donate the first month’s savings to the Node.js Foundation.

— via

;-)

sirreal · 2017-10-31T12:10:13Z

The preshrinkwrap script seems to create an infinite loop. I'd speculate that since we're trying to shrinkwrap and prune fiddles with node_modules, the process starts over. I have no evidence to support that, but removal of the preshrinkwrap script allows update-deps to complete.

I've removed it in 0ef64ba, any reasons to keep it?

blowery · 2017-10-31T13:30:35Z

+1 on moving to package.lock for locking.

sirreal · 2017-11-13T21:54:28Z

As far as I can see, there aren't any blockers and testing looks good. I'll plan to merge in the next 24 hours if there aren't any objections.

gwwar · 2017-11-13T22:16:30Z

Looks like most of the added lines are from the integrity field. If our first attempt doesn't build properly we may need to check if our cached packages match. https://docs.npmjs.com/files/package-lock.json#integrity

gwwar · 2017-11-13T22:47:23Z

Smoke tested a bit on Docker, and things appeared to work well ✨

I do see some issues with wp-desktop, but I think we can proceed with this and fix wp-desktop on that side.

@astralbodies are you okay with this approach? Or should we try fixing some of the issues now?

sirreal · 2017-11-14T13:01:32Z

astralbodies are you okay with this approach? Or should we try fixing some of the issues now?

I got an off-GitHub OK from astralbodies. Merge is imminent…

Don't run "npm install" twice, this was workaround for older npm peculiarities. Remove the `--dev` flag from `npm shrinkwrap` as it's now the default.

We'll continue to use `npm-shrinkwrap.json` which serves the same purpose.

Optional dependencies will continue to be installed if supported. This will generate the same shrinkwrap independent of the operating system where the shrinkwrap is generated. This was an annoyance where `fsevents` would be added or removed from the shrinkwrap frequently.

Change in 01dee67 appears to break docker builds on osx

It's used by `bin/minify.js`, which is called as part of production builds after `npm install --production`. This was breaking local docker builds.

* Revert #19317 * Regenerate shrinkwrap with node6/npm3

Applies the changes from #19317

sirreal · 2017-11-14T15:43:50Z

Build failed: pMz3w-7Pz-p2

Take 2: #19759

Upgrade to node 8 and npm 5 Second attempt with the changes from #19317

* Update to node 8.9.1 * Remove preshrinkwrap which was causing infinite loop * Simplified update-deps script for npm@5 Don't run "npm install" twice, this was workaround for older npm peculiarities. Remove the `--dev` flag from `npm shrinkwrap` as it's now the default. * Disable package-lock.json generation in config We'll continue to use `npm-shrinkwrap.json` which serves the same purpose. * Remove duplicate npm install from Dockerfile * Move uglify-js to dependencies It's used by `bin/minify.js`, which is called as part of production builds after `npm install --production`. This was breaking local docker builds.

* Revert Automattic#19317 * Regenerate shrinkwrap with node6/npm3

Upgrade to node 8 and npm 5 Second attempt with the changes from Automattic#19317

sirreal added Build Framework [Type] Task labels Oct 31, 2017

sirreal self-assigned this Oct 31, 2017

sirreal requested review from ockham, gwwar and DanReyLop October 31, 2017 10:02

sirreal changed the title ~~Update/node 8 8 1~~ Build: update Node to 8.8.1 Oct 31, 2017

sirreal added [Status] Blocked / Hold and removed [Status] In Progress labels Oct 31, 2017

sirreal added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Oct 31, 2017

sirreal force-pushed the update/node-8-8-1 branch from 79e1c10 to fce8116 Compare October 31, 2017 11:12

sirreal removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 13, 2017

sirreal and others added 10 commits November 14, 2017 14:02

Update to node 8.9.1

edeb0f9

Remove preshrinkwrap which was causing infinite loop

bcc52f9

Simplified update-deps script for npm@5

b395c08

Don't run "npm install" twice, this was workaround for older npm peculiarities. Remove the `--dev` flag from `npm shrinkwrap` as it's now the default.

Disable package-lock.json generation in config

e1db600

We'll continue to use `npm-shrinkwrap.json` which serves the same purpose.

Remove duplicate npm install from Dockerfile

0632304

Revert removal of optional deps from shrinkwrap

06e28b5

Change in 01dee67 appears to break docker builds on osx

Move uglify-js to dependencies

02dcba3

It's used by `bin/minify.js`, which is called as part of production builds after `npm install --production`. This was breaking local docker builds.

Remove retry description

5fb88bd

Regenerate shrinkwrap

95c7126

sirreal force-pushed the update/node-8-8-1 branch from 1ba5907 to 95c7126 Compare November 14, 2017 13:09

sirreal merged commit 04e5fde into master Nov 14, 2017

matticbot removed the [Status] Ready to Merge label Nov 14, 2017

sirreal added a commit that referenced this pull request Nov 14, 2017

Revert #19317

d967262

sirreal mentioned this pull request Nov 14, 2017

Build: Revert node 8 upgrade (#19317) #19756

Merged

sirreal added a commit that referenced this pull request Nov 14, 2017

Build: Revert node 8 upgrade (#19317) (#19756)

b10d346

* Revert #19317 * Regenerate shrinkwrap with node6/npm3

sirreal added a commit that referenced this pull request Nov 14, 2017

Upgrade to node 8 and npm 5

62a7a03

Applies the changes from #19317

sirreal mentioned this pull request Nov 14, 2017

Build: Upgrade to Node 8 LTS Carbon (take 2) #19759

Merged

sirreal added a commit that referenced this pull request Nov 15, 2017

Build: Upgrade to Node 8 LTS Carbon (#19759)

983d097

Upgrade to node 8 and npm 5 Second attempt with the changes from #19317

sirreal deleted the update/node-8-8-1 branch November 15, 2017 09:21

rclations pushed a commit to rclations/wp-calypso that referenced this pull request Nov 15, 2017

Build: Revert node 8 upgrade (Automattic#19317) (Automattic#19756)

5da0884

* Revert Automattic#19317 * Regenerate shrinkwrap with node6/npm3

rclations pushed a commit to rclations/wp-calypso that referenced this pull request Nov 15, 2017

Build: Upgrade to Node 8 LTS Carbon (Automattic#19759)

cbf6fea

Upgrade to node 8 and npm 5 Second attempt with the changes from Automattic#19317

blowery mentioned this pull request Mar 16, 2018

Framework: Avoid churn in npm-shrinkwrap.json #23348

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build: update Node to version 8 LTS (carbon) #19317

Build: update Node to version 8 LTS (carbon) #19317

sirreal commented Oct 31, 2017 •

edited

Loading

matticbot commented Oct 31, 2017

sirreal commented Oct 31, 2017

sirreal commented Oct 31, 2017

DanReyLop commented Oct 31, 2017

sirreal commented Oct 31, 2017

sirreal commented Oct 31, 2017 •

edited

Loading

jsnajdr commented Oct 31, 2017

gziolo commented Oct 31, 2017

sirreal commented Oct 31, 2017 •

edited

Loading

simison commented Oct 31, 2017 •

edited

Loading

simison commented Oct 31, 2017 •

edited

Loading

sirreal commented Oct 31, 2017

blowery commented Oct 31, 2017

sirreal commented Nov 13, 2017 •

edited

Loading

gwwar commented Nov 13, 2017

gwwar commented Nov 13, 2017

sirreal commented Nov 14, 2017 •

edited

Loading

sirreal commented Nov 14, 2017

Build: update Node to version 8 LTS (carbon) #19317

Build: update Node to version 8 LTS (carbon) #19317

Conversation

sirreal commented Oct 31, 2017 • edited Loading

Testing

Blockers

matticbot commented Oct 31, 2017

sirreal commented Oct 31, 2017

sirreal commented Oct 31, 2017

DanReyLop commented Oct 31, 2017

sirreal commented Oct 31, 2017

sirreal commented Oct 31, 2017 • edited Loading

jsnajdr commented Oct 31, 2017

gziolo commented Oct 31, 2017

sirreal commented Oct 31, 2017 • edited Loading

simison commented Oct 31, 2017 • edited Loading

simison commented Oct 31, 2017 • edited Loading

sirreal commented Oct 31, 2017

blowery commented Oct 31, 2017

sirreal commented Nov 13, 2017 • edited Loading

gwwar commented Nov 13, 2017

gwwar commented Nov 13, 2017

sirreal commented Nov 14, 2017 • edited Loading

sirreal commented Nov 14, 2017

sirreal commented Oct 31, 2017 •

edited

Loading

sirreal commented Oct 31, 2017 •

edited

Loading

sirreal commented Oct 31, 2017 •

edited

Loading

simison commented Oct 31, 2017 •

edited

Loading

simison commented Oct 31, 2017 •

edited

Loading

sirreal commented Nov 13, 2017 •

edited

Loading

sirreal commented Nov 14, 2017 •

edited

Loading