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.

Sign up for GitHub

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

Jump to bottom

build(node): upgrade dev setup to Node v12 #4088

Merged
merged 1 commit into from
Sep 23, 2019
Merged

build(node): upgrade dev setup to Node v12 #4088

merged 1 commit into from
Sep 23, 2019

Conversation

mcous
Copy link
Contributor

@mcous mcous commented Sep 19, 2019 •
edited
Loading

overview

  • Our currently spec'd version of Node in the monorepo is v8 (lts/carbon), which is scheduled for end-of-life at the end of the year
  • This only affects our development environments. Our only production use of Node in the monorepo is in app-shell, and that Node version is driven by electron
  • Our currently deployed electron version in the app uses Node v12, which is scheduled for LTS in October

This PR upgrades our dev environment to v12

Blocked by #4083 because it upgrades a bunch of our tools with changes to support Node v10 and v12 4083 merged and this PR has been rebased

changelog

  • build(node): upgrade dev setup to Node v12

review requests

If using nvm, checkout this branch and:

nvm install
nvm use
node --version
make install-js

Otherwise, get Node v12 on your machine somehow, ensure node --version returns 12, run make install-js, and make sure everything still builds and runs.

If you encounter any weird yarn failures, you can try:

  • Removing all your node_modules folders and retrying
  • Removing your node-gyp cache, which should be in the default OS-specific cache location
    • On macOS, this is ~/Library/Caches/node-gyp

@mcous mcous added chore ready for review blocked Ticket or PR is blocked by other work labels Sep 19, 2019
@mcous mcous requested review from a team September 19, 2019 19:03
@mcous mcous changed the base branch from chore_browserslist to edge September 20, 2019 14:04
@codecov
Copy link

codecov bot commented Sep 20, 2019 •
edited
Loading

Codecov Report

Merging #4088 into edge will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             edge    #4088   +/-   ##
=======================================
  Coverage   57.24%   57.24%           
=======================================
  Files         850      850           
  Lines       24002    24002           
=======================================
  Hits        13741    13741           
  Misses      10261    10261

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33a4012...7fb02ae. Read the comment docs.

@mcous mcous removed the blocked Ticket or PR is blocked by other work label Sep 20, 2019
sfoster1
sfoster1 approved these changes Sep 20, 2019
View reviewed changes
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

make install and make lint test all succeed on my machine, not sure if there's more testing to do

Kadee80
Kadee80 approved these changes Sep 23, 2019
View reviewed changes
Copy link
Contributor

@Kadee80 Kadee80 left a comment

Choose a reason for hiding this comment

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

⬆️

  • App builds
  • LL/LC builds
  • PD builds

IanLondon
IanLondon approved these changes Sep 23, 2019
View reviewed changes
Copy link
Contributor

@IanLondon IanLondon left a comment

Choose a reason for hiding this comment

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

Tested PD, LL/LC, and App builds, all good. Tested PD and LL sandbox links, they work with no weird console errors or anything

IanLondon
IanLondon reviewed Sep 23, 2019
View reviewed changes
CONTRIBUTING.md

```shell
nvm install lts/carbon
nvm install 12
Copy link
Contributor

Choose a reason for hiding this comment

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

should this just be nvm install?

Copy link
Contributor Author

@mcous mcous Sep 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

That will work if the user is in the opentrons directory, but the instruction to clone the repository doesn't come until later in this doc. We definitely need to expand and rework this section for better OS-specific instructions, so that's a good thing to keep in mind

btmorr
btmorr approved these changes Sep 23, 2019
View reviewed changes
Copy link
Contributor

@btmorr btmorr left a comment

Choose a reason for hiding this comment

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

🍡 Install, test, and lint pass on my laptop (OSX)

b-cooper
b-cooper approved these changes Sep 23, 2019
View reviewed changes
Copy link
Contributor

@b-cooper b-cooper left a comment

Choose a reason for hiding this comment

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

🐰

@mcous mcous merged commit fd2ad8f into edge Sep 23, 2019
@mcous mcous deleted the build_node-12 branch September 23, 2019 21:19
@mcous mcous mentioned this pull request Sep 25, 2019
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IanLondon IanLondon IanLondon approved these changes

@btmorr btmorr btmorr approved these changes

@b-cooper b-cooper b-cooper approved these changes

@Kadee80 Kadee80 Kadee80 approved these changes

@sfoster1 sfoster1 sfoster1 approved these changes

Assignees
No one assigned
Labels
chore
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mcous @sfoster1 @Kadee80 @b-cooper @btmorr @IanLondon