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.

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

Release gh-badges to npm #1388

Closed
felixfbecker opened this issue Dec 23, 2017 · 19 comments
Closed

Release gh-badges to npm #1388

felixfbecker opened this issue Dec 23, 2017 · 19 comments
Labels
core Server, BaseService, GitHub auth, Shared helpers

Comments

@felixfbecker
Copy link
Contributor

The library readme on npm looks incredibly outdated and the examples in there don't work:

var badge = require('gh-badges');
badge({ text: [ "build", "passed" ], colorscheme: "green" },
  function(svg) {
    // svg is a String… of your badge.
  });

throws an error because template is not set (which is mentioned in the README on master).

@paulmelnikow paulmelnikow changed the title Readme on npm outdated Release gh-badges to npm Dec 24, 2017
@paulmelnikow paulmelnikow added the core Server, BaseService, GitHub auth, Shared helpers label Dec 24, 2017
@paulmelnikow
Copy link
Member

Yea, agreed, it would be a good idea to do a release. There are a few things that need doing:

  • Update the documentation in the readme (and document the API in doc/ if needed)
  • Make sure the public interface has tests
  • Make sure we're publishing the files we need
  • Publish a 2.0 RC

@felixfbecker
Copy link
Contributor Author

What are the breaking changes for 2.0?

@paulmelnikow
Copy link
Member

  1. makeBadge has a new interface which is synchronous.
  2. There's a new text measurer that needs to be passed in (see Speed up font-width computation in most cases #1390) which is also synchronous.
  3. For consumers of the library, we probably should add a convenience function which memoizes the measurer and avoids boilerplate in code which consumes it.
  4. The CLI should be the same.

@felixfbecker
Copy link
Contributor Author

How far away are we from this? Maybe we could get incremental improvements instead of blocking on all these changes if it delays the release so long? I.e. if there is another breaking change, just cut 3.0, version numbers are cheap.

@stclairdaniel
Copy link

I'm curious about this too - 1.3.0 has a lot of security vulnerabilities and I'd like to upgrade.

@chris48s
Copy link
Member

Looking over the tasks, I don't mind having a look at this job. I assume when you say "make sure we're publishing the files we need", you mean ensuring this is right:

shields/package.json

Lines 101 to 111 in 9b6ba77

"files": [
"README.md",
"lib/badge-cli.js",
"lib/make-badge.js",
"lib/colorscheme.json",
"lib/lru-cache.js",
"lib/text-measurer.js",
"lib/svg-to-img.js",
"templates",
"logo"
],

@paulmelnikow - if I can spend a bit of time to get the docs into shape, check the tests and test a package, do you have access to npm publish it?

@chris48s
Copy link
Member

I've submitted PR #2200 with some changes supporting this. It isn't an aspect of the project I've given any attention to before, but having done a bit of work on this, I think there are several other jobs we should do to improve the NPM package.

There is one issue I've raised in #2200 which I think we should consider before publishing a 2.0.0 release (should we force the user to make a choice of text measurer in order to generate a badge?). Additionally, these shouldn't be blockers to a 2.0.0 but we should think about them for future:

  • There is some internal code which we currently need to include in the NPM library due to tight internal coupling which isn't strictly necessary for NPM package users. e.g: the LRU cache module.
  • Because shields.io and gh-badges share the same package.json, our NPM package users are inheriting a load of unnecessary dependencies which are needed by shields.io but not the NPM package.
  • It is difficult to provide a good readme for NPM because npm pack collects the README.md from the root: https://docs.npmjs.com/files/package.json#files There isn't a neat way to specify "replace README.md with doc/gh-badges.md when we run npm pack".
  • Our package users also inherit a bunch on NPM scripts which aren't relevant to the library.

Those issues all stem from the fact we are trying to break a package out of a monolith. IMO we should aim to treat the subset of our codebase which makes up the gh-badges package as a package in its own right which shields.io depends on. It could either live in its own repo under the @badges org or use a "monorepo" arrangement with the package living in a separate corner of the repo with its own package.json. That would allow us to solve several of these issues. It would also create a situation where we're "eating our own dogfood", which would provide a much greater incentive to exercise better stewardship over the package.

@felixfbecker
Copy link
Contributor Author

+1 for putting the package into a separate repo.

We may be able to do this post-2.0 though. Lets get 2.0 released as soon as possible.

@chris48s
Copy link
Member

Think we've got the details nailed down in #2200 now, but if I merge it I don't have permission to npm publish the package. @espadrine , @paulmelnikow can either of you tag in to help get a 2.0.0-beta out?

@paulmelnikow
Copy link
Member

I do not have access to the npm package. I'll email Thaddée. Can you confirm you're https://www.npmjs.com/~chris48s on npm?

Meanwhile let's not block. We could either publish to a scoped name like @shields/gh-badges and then roll things back into the main gh-badges when we get access, or pick a more meaningful name and plan to retire gh-badges. There's a way to mark old packages as not supported or provide a message encouraging users to switch. I haven't used it before, but I've seen those messages.

"github badges" is not really what this package does… so perhaps make-badge?

@paulmelnikow
Copy link
Member

P.S. Thank you so much @chris48s for taking this on!

@paulmelnikow
Copy link
Member

👍 on a separate package.json for the library. I'd lean monorepo to make for a simpler test and release process for the production server. Though if we can get things better decoupled, and this into what feels like a "finished" state, I'd feel better about breaking it out. Checking in a Verdana character width table would be good, too, since it would remove the dependence on Verdana.ttf and resolve #1309.

@chris48s
Copy link
Member

I had also thought about publishing a new package name if we need to. Did you already snag https://www.npmjs.com/org/shields or is that somebody else?

@paulmelnikow
Copy link
Member

Nooope, that is somebody else…

@paulmelnikow
Copy link
Member

Though there’s no need! Thaddee has added both of us to the gh-badges package. (I didn’t get a notification, and I’m not appearing on the public page, though it does show up in my list of packages.)

@chris48s
Copy link
Member

Sorry it has taken a while to get this sorted, but I've just published 2.0.0-beta1 to NPM:

https://www.npmjs.com/package/gh-badges/v/2.0.0-beta1

It would be useful to get a bit of feedback from those keen to get hold of a new release at this point. If there are no showstoppers, I'll cut a 2.0.0 stable.

Having done a bit of work on this, there are clearly some additional improvements we can make, but they can be done in minor releases without BC-breaks. Now that I have access to publish it to NPM, we should be able to work on incremental improvements more easily :)

@paulmelnikow
Copy link
Member

Really glad to have a beta shipped! Should we open a new issue for the follow-ons, like the monorepo / repo splitting?

By the way, we've removed all the vulnerable dependencies as of latest master, which is a big improvement compared to the beta1 release.

@chris48s
Copy link
Member

chris48s commented Nov 6, 2018

Yep - nobody seems to have raised any major issues so I think in the next few days we should:

  • Release 2.0.0 without any substantial changes from the beta
  • Close this issue
  • Track further work in new issues
  • Put out more 2.x releases as we restructure internals

@chris48s
Copy link
Member

chris48s commented Nov 9, 2018

closing this as v2.0.0 is now published.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Server, BaseService, GitHub auth, Shared helpers
Projects
None yet
Development

No branches or pull requests

4 participants