Skip to content
This repository was archived by the owner on Sep 3, 2022. It is now read-only.

Replace robo with np for releasing. #96

Merged
merged 1 commit into from
Jul 9, 2018
Merged

Replace robo with np for releasing. #96

merged 1 commit into from
Jul 9, 2018

Conversation

f2prateek
Copy link
Contributor

np is nice for a few things:

  • stricter checks (e.g. can't publish from a branch without forcing)
  • human readable version bumps (e.g. minor, major, patch)
  • supports distribution tags
  • public (doesn't require our private robofiles)
  • more user friendly UI

The one disadvantage is that it publishes directly to NPM locally, so we'll need to use the --no-publish flag to avoid releasing from our local dev machines and have the CI do it instead (following https://github.com/sindresorhus/np#publish-with-a-ci). I added a helper script that has this flag set by default to make it easy to run as well.

np is nice for a few things:
* stricter checks (e.g. can't publish from a branch without forcing)
* human readable version bumps (e.g. minor, major, patch)
* supports distribution tags
* public (doesn't require our private robofiles)
* more user friendly UI

The one disadvantage is that it publishes directly to NPM locally, so we'll need to use the `--no-publish` flag to avoid releasing from our local dev machines and have the CI do it instead (following https://github.com/sindresorhus/np#publish-with-a-ci). I added a helper script that has this flag set by default to make it easy to run as well.
@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #96   +/-   ##
=======================================
  Coverage   98.58%   98.58%           
=======================================
  Files          11       11           
  Lines         636      636           
=======================================
  Hits          627      627           
  Misses          9        9

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 0e168a5...7c1f1b9. Read the comment docs.

@ucarion
Copy link

ucarion commented Jul 6, 2018

I think I'm opposed to this change. I fear np will ultimately be vaporware, and that by cutting over to this tool we're adding complexity to how analytics.js is deployed. npm is a rather well-understood, widely-used tool. np is not.

@f2prateek
Copy link
Contributor Author

f2prateek commented Jul 6, 2018

I don't think we were using npm at all previously for this step? NPM was only used for the CI releasing step, and it will continue to be used for that part.

Here I'm replacing our local release steps using robo with np mostly for the reasons I mentioned in #96 (comment):

np is nice for a few things: 

* stricter checks (e.g. can't publish from a branch without forcing)
* human readable version bumps (e.g. minor, major, patch) --- this one applies to robo too though
* supports distribution tags
* public (doesn't require our private robofiles)
* more user friendly UI

I think robo (with < 500 stars) is likely to out of fashion before np (> 3k stars).

@ucarion
Copy link

ucarion commented Jul 6, 2018

I agree that having robo mediate releases is non-ideal. It makes Analytics.js seem less open when it relies on tooling that only Segment has access to.

But I feel that using yarn or npm directly remains a better option. We use a lot of tools as it is. Is this change part of a broader effort at improving our tooling? Because I fear that one-off PRs aren't going to help us have a coherent story around how we do releases.

If analytics.js-core is going to be the only place where we use np, then I would prefer we not use it at all.

@vadimdemedes
Copy link

vadimdemedes commented Jul 6, 2018

I think I'm opposed to this change. I fear np will ultimately be vaporware, and that by cutting over to this tool we're adding complexity to how analytics.js is deployed. npm is a rather well-understood, widely-used tool. np is not.

I disagree, np is a widely used tool for publishing npm packages. It's created by one of the top JS developers in the world - https://github.com/sindresorhus. He uses np to publish hundreds of his own packages. It's also the safest way to publish a package to npm, see https://github.com/sindresorhus/np#why.

@f2prateek huge 👍 on this change!

@f2prateek
Copy link
Contributor Author

f2prateek commented Jul 6, 2018

@ucarion I think the main goals here are to make releasing analytics.js-core simpler and safer (and not necessarily the full a.js process). I don't feel yarn or npm give that out of the box, and np adds enough value to make the case to use it here.

Specifically around the safety piece, np does the following that we don't get with NPM/Yarn out of the box and would have to wire up ourselves:

  • Ensures you are publishing from the master branch
  • Ensures the working directory is clean and that there are no unpulled changes
  • Reinstalls dependencies to ensure your project works with the latest dependency tree
  • Runs the tests
  • Prevents accidental publishing of pre-release versions under the latest dist-tag

It doesn't feel like we're really coupling ourselves to np here, so in the case np was to go away, we could give up this safety and use npm/yarn as you mentioned (or even go back to hand editing files + robo).

We do already use np in analytics-node so it feels less non-standardized to me, but I can see where you're coming from since we don't use this in a.js integrations yet. I also do think a little bit of deviation between a.js and a.js integrations is ok though (e.g. we already use Yarn here but not in a.js integrations yet).

@ucarion
Copy link

ucarion commented Jul 6, 2018

Ok, sounds good. My objection was always a tepid one, and I think you've made a strong case for np. Indeed, we're not really that coupled to it, and the fact that there's precedent helps too.

Consider me 👍 ! I've never made changes to this repo though, so I'll hold off on approving the PR.

@f2prateek f2prateek requested a review from Flaque July 7, 2018 15:51
@f2prateek f2prateek merged commit 1187fc3 into master Jul 9, 2018
@f2prateek f2prateek deleted the np branch July 9, 2018 17:14
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.

6 participants