Framework: Move to Node 4 #1204

blowery · 2015-12-02T22:03:18Z

This moves us up to Node v4.2.3, the current LTS version, in the Docker container. This needs a fair bit of testing and should not be merged until we're pretty happy with it.

Yes, the branch name is wrong.

To test, you'll need to

install Vagrant,
add a env-config.sh to the root of the project which exports CALYPSO_ENV set to a environment you want to test
run vagrant up
add the correct entries to /etc/hosts to hit your local env for the host you want to pretend to be
add in a forwarder from port 80 to port 3000 locally, using nginx or polipo

TODO:

Bump Dockerfile up to 4.2.3
Add circle.yml to tell Circle CI which node to use
Upgrade any packages that need updates to run well on 4
Fix up tests
Bump engines in package.json
Add Makefile target that checks node version and explodes if the dev is on pre 4.2.3.

ehg · 2015-12-03T18:33:27Z

It would be cool to merge #730 to avoid some duplication.

I will give 4.4.2 a test with vagrant. With regards to merging this, I think a similar approach to the webpack server bundler is a good idea. e.g. notify systems, disable deploys to production for 30 minutes.

blowery · 2015-12-03T20:09:18Z

One little wrinkle I ran into: on Node v4+, it appears that the default http listener only listens on an IPv6 loopback, if it's available. If not, it falls back to the normal 127.0.0.1 IPv4 address. I had to update my /etc/hosts to point calypso.localhost at ::1 instead of 127.0.0.1 to make things work as expected.

blowery · 2015-12-03T20:30:59Z

Bah, I transposed the node version on the branch name and the commit. Should be 4.2.2, not 4.4.2.

Fixing the commit.

blowery · 2015-12-03T21:24:59Z

Redid this on top of master now that #730 landed

blowery · 2015-12-06T17:49:48Z

Hmmmm... So switching Circle CI over to node 4.2.2 breaks the build, even if built with a clean cache. It appears to be breaking on contextify? Maybe jumping up to a current jsdom would help there. It's pretty disconcerting to see all the build errors.

ehg · 2015-12-07T12:21:06Z

Maybe jumping up to a current jsdom would help there. It's pretty disconcerting to see all the build errors.

@sirbrillig I think you mentioned the latest jsdom doesn't need contextify?

sirbrillig · 2015-12-07T16:04:56Z

Yup, that's correct! Relevant comment.

3.0.x will be the last release of jsdom to support Node.js. All future releases (starting with 4.0.0) will require io.js, whose new vm module will allow us to remove our contextify native-module dependency. (Given that I submitted the relevant patch to joyent/node 1.5 years ago, I'm very excited that we can finally use it!)

Also this:

This release relies on the newly-overhauled vm module of io.js to eliminate the Contextify native module dependency. jsdom should now be much easier to use and install, without requiring a C++ compiler toolchain!

ehg · 2015-12-07T16:10:14Z

So we'd have to get everyone to upgrade to node 4+ on their local machines… :)

aduth · 2015-12-07T19:35:49Z

client/my-sites/upgrades/cart/test/test-cart-buttons.jsx

@@ -3,7 +3,7 @@ require( 'lib/react-test-env-setup' )();
 /**
 * External Dependencies
 */
-var	sinon = require( 'sinon' ),
+const	sinon = require( 'sinon' ),


Does this change need to be made? If we're changing things, I'd rather see this go to import syntax than to const.

it was mostly to quiet up eslint.

gwwar · 2015-12-07T22:35:49Z

Don't forget to update the node engine version to our minimum supported version in package.json:

"engines": {
    "node": ">=4.2.3"
}

blowery · 2015-12-08T00:10:51Z

Don't forget to update the node engine version to our minimum supported version in package.json

Done. Interesting reading at http://www.marcusoft.net/2015/03/packagejson-and-engines-and-enginestrict.html. Looks like it's mostly advisory since we're the app.

gwwar · 2015-12-08T00:54:47Z

Yup, it sadly only warns you if your package is installed as a dependency, but as a node developer it's still the first place I check for version compatibility.

sirbrillig · 2015-12-08T16:18:38Z

A version check for Node in the Makefile might make sense, as suggested elsewhere by @jkudish. Something like this as a target:

node-version:
    @if [ "$(shell $(NODE) --version | sed 's/[^0-9]//g')" -lt 400 ]; then echo "Please upgrade your version of Node.js: https://nodejs.org/"; exit 1; fi

blowery · 2015-12-08T17:54:54Z

A version check for Node in the Makefile might make sense,

I added this a slightly different way, using a node script and the semver property in package. This way it could work on Windows too, and we have a single source of truth for the necessary node version.

sirbrillig · 2015-12-08T18:05:10Z

using a node script and the semver property in package

That's way better! Awesome. 👍

blowery · 2015-12-08T18:09:16Z

Except semver isn't baked in... sigh.

aduth · 2015-12-08T19:06:35Z

I force-pushed a rebase to account for tests added in #550 which would have broken after the upgrade here (fixup'd into b81e2de).

I also fixed the version compare script to pull the semver dependency from the npm global root node_modules directory. The semver package is not bundled with Node, but is declared as a dependency of and maintained by the folks behind npm (9f8d852). This feels a bit hacky, but works well in my testing.

aduth · 2015-12-14T16:26:54Z

Should we add a project-wide .nvmrc ? Would that be useful?

Sounds useful, though it would appear that we'd need to update Make targets to use nvm exec if nvm is installed? Personally, I use n, so I wouldn't benefit from this.

blowery · 2015-12-15T15:45:58Z

Sounds useful, though it would appear that we'd need to update Make targets to use nvm exec if nvm is installed?

Nah, it just makes nvm use easier. No reason to force nvm exec into the Makefile.

blowery · 2015-12-15T18:45:11Z

Hrm, serving / and the CSS seems much slower now. I'm seeing the CSS rebuild on every request... Digging.

blowery · 2015-12-15T19:11:08Z

Hrm, serving / and the CSS seems much slower now. I'm seeing the CSS rebuild on every request...

This was due to the new node-version target. Made it order-only in 338e3e9 which prevents it from causing a rebuild on every request.

Also fix the duped styles in follow-button.

See https://github.com/nodejs/node/blob/v4.2.3/CHANGELOG.md#2015-12-04-version-423-argon-lts-rvagg

The URL spec lets it resolve relative URLs without exploding. I also turned off resource loading to prevent any accidental requests from going out when using JSDom. Last, expose the history object for page.js to use.

Since `semver` is not default to node but is included in any npm installation

This allows the dependency to run, but not foul the dep tree. With it as a normal dep, it would cause any target that depended upon it to re-run, every time it was invoked. This made a target like build-css rerun for every http request, which slowed things down considerably. See https://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html for more info on order-only deps

blowery · 2015-12-17T18:49:34Z

Just got this up and running, pretending to be a production node using my local setup, and it seems to work just fine. I think this is ready to go.

ehg · 2015-12-17T19:33:31Z

I think it's good to go too, works fine for me 👍

Framework: Move to Node 4

Somehow I broke this with the last commit on #1204 and didn't notice. This re-instates the call to `check-node-version` to inform folks on old nodes that they need to upgrade

blowery added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 2, 2015

blowery self-assigned this Dec 2, 2015

blowery force-pushed the try/node-4.4.2 branch 2 times, most recently from 1d3d5a5 to d3be21a Compare December 3, 2015 21:24

blowery added the Build label Dec 3, 2015

sirbrillig mentioned this pull request Dec 4, 2015

Update node-v0.12.6 to v0.12.8 #719

Closed

blowery force-pushed the try/node-4.4.2 branch from 81e0012 to 7eebb33 Compare December 6, 2015 16:55

ehg force-pushed the try/node-4.4.2 branch 2 times, most recently from defde4b to a7e44e1 Compare December 7, 2015 15:37

blowery force-pushed the try/node-4.4.2 branch from a7e44e1 to 03c6ef5 Compare December 7, 2015 16:08

blowery force-pushed the try/node-4.4.2 branch from 98f2c9d to 2ce2a1d Compare December 7, 2015 17:38

aduth reviewed Dec 7, 2015
View reviewed changes

aduth force-pushed the try/node-4.4.2 branch from bdd073b to 9f8d852 Compare December 8, 2015 19:06

blowery force-pushed the try/node-4.4.2 branch from 5767b7b to e2ee3d1 Compare December 14, 2015 15:27

blowery changed the title Framework: Move to Node 4.2.2 ** DO NOT MERGE ** Framework: Move to Node 4 Dec 15, 2015

blowery and others added 9 commits December 15, 2015 15:24

Framework: Move to Node 4.2.2

cd30470

Build: Tell Circle CI to use node 4.2.2

7e5f147

Fixup some tests that broke with jsdom 7.

ae7d8a5

Also fix the duped styles in follow-button.

Node 4.2.3, 4.2.2 has a sec bug

46336e2

See https://github.com/nodejs/node/blob/v4.2.3/CHANGELOG.md#2015-12-04-version-423-argon-lts-rvagg

Make JSDom a bit safer and give it a base URL.

8c3421d

The URL spec lets it resolve relative URLs without exploding. I also turned off resource loading to prevent any accidental requests from going out when using JSDom. Last, expose the history object for page.js to use.

Mark wp-calypso as needing node 4.2.3 or higher

7f192a0

Add a check to verify that the node version is proper

96c1c53

Framework: Pass global npm root to require semver

800f4ae

Since `semver` is not default to node but is included in any npm installation

blowery force-pushed the try/node-4.4.2 branch from 338e3e9 to 6b0fb9c Compare December 15, 2015 20:24

blowery added a commit that referenced this pull request Dec 17, 2015

Merge pull request #1204 from Automattic/try/node-4.4.2

66126f3

Framework: Move to Node 4

blowery merged commit 66126f3 into master Dec 17, 2015

blowery deleted the try/node-4.4.2 branch December 17, 2015 19:40

scruffian removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 17, 2015

blowery added a commit that referenced this pull request Dec 18, 2015

Build: fix the node-version rule.

3a358d1

Somehow I broke this with the last commit on #1204 and didn't notice. This re-instates the call to `check-node-version` to inform folks on old nodes that they need to upgrade

blowery mentioned this pull request Dec 18, 2015

Build: fix the node-version rule. #1819

Merged

snyk-bot mentioned this pull request Apr 14, 2021

[Snyk] Fix for 1 vulnerabilities awaisab172/wp-calypso#66

Open

This was referenced Apr 27, 2021

[Snyk] Fix for 1 vulnerabilities awaisab172/wp-calypso#72

Open

[Snyk] Fix for 2 vulnerabilities ajesse11x/wp-calypso#48

Open

jgcaruso mentioned this pull request May 31, 2022

VideoPress: Enable "Video Privacy" field in Media Library for Jetpack sites #64210

Merged

awaisab172 mentioned this pull request Oct 1, 2023

[Snyk] Fix for 1 vulnerabilities awaisab172/wp-calypso#157

Open

ajesse11x mentioned this pull request Oct 1, 2023

[Snyk] Fix for 1 vulnerabilities ajesse11x/wp-calypso#92

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Framework: Move to Node 4 #1204

Framework: Move to Node 4 #1204

blowery commented Dec 2, 2015

ehg commented Dec 3, 2015

blowery commented Dec 3, 2015

blowery commented Dec 3, 2015

blowery commented Dec 3, 2015

blowery commented Dec 6, 2015

ehg commented Dec 7, 2015

sirbrillig commented Dec 7, 2015

ehg commented Dec 7, 2015

aduth Dec 7, 2015

blowery Dec 7, 2015

gwwar commented Dec 7, 2015

blowery commented Dec 8, 2015

gwwar commented Dec 8, 2015

sirbrillig commented Dec 8, 2015

blowery commented Dec 8, 2015

sirbrillig commented Dec 8, 2015

blowery commented Dec 8, 2015

aduth commented Dec 8, 2015

aduth commented Dec 14, 2015

blowery commented Dec 15, 2015

blowery commented Dec 15, 2015

blowery commented Dec 15, 2015

blowery commented Dec 17, 2015

ehg commented Dec 17, 2015

Framework: Move to Node 4 #1204

Framework: Move to Node 4 #1204

Conversation

blowery commented Dec 2, 2015

ehg commented Dec 3, 2015

blowery commented Dec 3, 2015

blowery commented Dec 3, 2015

blowery commented Dec 3, 2015

blowery commented Dec 6, 2015

ehg commented Dec 7, 2015

sirbrillig commented Dec 7, 2015

ehg commented Dec 7, 2015

aduth Dec 7, 2015

Choose a reason for hiding this comment

blowery Dec 7, 2015

Choose a reason for hiding this comment

gwwar commented Dec 7, 2015

blowery commented Dec 8, 2015

gwwar commented Dec 8, 2015

sirbrillig commented Dec 8, 2015

blowery commented Dec 8, 2015

sirbrillig commented Dec 8, 2015

blowery commented Dec 8, 2015

aduth commented Dec 8, 2015

aduth commented Dec 14, 2015

blowery commented Dec 15, 2015

blowery commented Dec 15, 2015

blowery commented Dec 15, 2015

blowery commented Dec 17, 2015

ehg commented Dec 17, 2015