Skip to content
This repository has been archived by the owner on Dec 13, 2018. It is now read-only.

tech(node): add node as a dep. #388

Merged
merged 2 commits into from
Mar 12, 2018
Merged

tech(node): add node as a dep. #388

merged 2 commits into from
Mar 12, 2018

Conversation

kentcdodds
Copy link
Contributor

What: Adds node as a dependency. This is a brilliant idea from the folks at npm. It allows us to have any version of node and as long as the folks using the project only use npm scripts to run our stuff, the installed version of node will be the one used!

Why: Closes #352

How: add node to the dev dependencies and update contributing docs to be a little simpler.

Checklist:

  • Documentation N/A
  • Tests N/A
  • Ready to be merged
  • Added myself to contributors table N/A

@codecov-io
Copy link

codecov-io commented Feb 21, 2018

Codecov Report

Merging #388 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #388   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          10     10           
  Lines         177    177           
  Branches       50     50           
=====================================
  Hits          177    177

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 ce401a8...711cf50. Read the comment docs.

luke-john
luke-john previously approved these changes Feb 21, 2018
Copy link
Collaborator

@luke-john luke-john left a comment

Choose a reason for hiding this comment

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

Looks great, love the idea of this.

The dependency is fairly large, but for development purposes I think the benefits are great and it should help keep glamorous easy to contribute to and maintain.

2. `$ npm install` to install dependencies
3. `$ npm run validate` to validate you've got it working
4. Create a branch for your PR
2. `npm run setup --silent` to setup the project (it installs deps and runs the validate script)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the --silent is needed here, that's already been added to the validate command under the setup script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, still need it to prevent npm from logging useless stuff when running the command I'm afraid.

package.json Outdated
@@ -56,6 +57,7 @@
"glamor": "^2.20.37",
"jest-glamor-react": "^3.2.2",
"kcd-scripts": "^0.31.1",
"node": "^8.9.4",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great idea adding this, hopefully it should make contributing a bit easier and cut down on issues around node versions 🙂.

@kentcdodds kentcdodds merged commit 29c5afd into master Mar 12, 2018
@kentcdodds kentcdodds deleted the pr/add-node branch March 12, 2018 22:11
@atticoos
Copy link
Collaborator

atticoos commented Mar 13, 2018

That's really cool

This is a brilliant idea from the folks at npm.

Is there a source you could share? Tried to find more information about this, but it's a tricky search term to research and didn't seem to find anything in https://docs.npmjs.com/files/package.json 😛

@kentcdodds
Copy link
Contributor Author

This is as official as I could get: https://twitter.com/maybekatz/status/958157474397171712

It's not an npm thing actually. It's just a normal npm package! Pretty interesting exercise to go through the code actually if you're curious. Start here: https://unpkg.com/node/

@atticoos
Copy link
Collaborator

atticoos commented Mar 13, 2018

Thanks!

Forgot to follow up and mention the README of https://www.npmjs.com/package/node pretty much sums it up

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

Successfully merging this pull request may close these issues.

npm run validate fails on current master
5 participants