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

Publishing to node #1

Open
quinn-dev opened this issue Jul 27, 2021 · 10 comments
Open

Publishing to node #1

quinn-dev opened this issue Jul 27, 2021 · 10 comments

Comments

@quinn-dev
Copy link
Contributor

First of all, I'm sorry for mostly abandoning the issue I opened in the old repository.

As you pointed out, a lot has changed since the end of January when I initially opened the issue.
One thing however remained mostly unchanged: There is no "practical" way of using the library in node.

What do I mean by practical? For better or worse, npm is the de facto standard for node dependency and version management. The vast, vast majority of node projects use npm. That's by no means limited to server-side projects - most modern frontend frameworks use and depend on npm modules. In other words: If you are starting any new JavaScript project using a modern stack, npm is more or less inevitable.

Now, when you find a good library - like this one - that you want to use in your npm-dependent project, you can't just npm install or yarn add it. Downloading the library and adding it to your project is more cumbersome yet still simple enough. But suddenly there's a moving part in your repository, which is managed completely different from the rest of your dependencies, and you need to keep track of it and update it.

So while it's currently not impossible to use this library in a npm-dependent project (read: most JS projects), it's a total pain in the ass to do so.

Good news: It doesn't have to be. It's very easy to publish this library to npm without touching the build system or anything else.

For this to work, the library needs to have a version number (I very strongly suggest following semver) and a name (I'd suggest @sleepdiary/core, see at the end of this issue).

All we would need to change in the repository is add a package.json that looks like this:

{
  "name": "@sleepdiary/core",
  "description": "A short and precise description",
  "version": "0.1.0",
  "main": "sleepdiary-library.min.js",
  "repository": "sleepdiary/library",
  "license": "MIT",
  "scripts": {
    "prepare": "docker run --rm -v \"$PWD\":/sleepdiary-library $(docker build -q .)"
  },
  "files": [
    "sleepdiary-library.min.js"
  ],
  "dependencies": {
    "timezonecomplete": "^5.12.4",
    "xmldom": "^0.6.0"
  }
}

That's essentially it.

In order to publish a new version of the module, you can either run npm publish, which will simply build the library, pack and publish it, or you can use something like np, an improved npm publish that also helps with versioning, release tagging, etc.

To revisit the issue of naming: I propose the syntax of @sleepdiary/<module>. The @ symbol indicates that the module belongs to the sleepdiary user (see npm scope). Moreover, the syntax allows the user to easily identify which modules belong to which project.

All three modules that are part of the sleepdiary project could be published this way, named @sleepdiary/core, @sleepdiary/info and @sleepdiary/report respectively. @sleepdiary/core is a personal suggestion, as the term library is quite ubiquitous in this context, but it could just as well be named @sleepdiary/library or anything else. Obviously, it makes sense to keep the names consistent.

To summarize: It's very little effort to implement and maintain this and it would allow for this library to be used in any npm-dependent project just as easily as any other module.

@andrew-sayers
Copy link
Contributor

No problem about the old issue - just glad you're back :)

I've opened a couple of discussions to deal with the wider issues - see Scope and name of different repositories and How best to support other developers. For now let's limit this issue to implementing an npm release for the library itself, on the assumption we go ahead with it.

I'm fine with semver for the library, although for the record we will also need to use the git commit ID in some places. In particular, the report is read by doctors who I'd expect to interpret a commit ID as gibberish they don't care about, but who might think "1.2.3" is some kind of annoying typo. Also, it's more important for that PDF to contain an automated guarantee that nothing's changed, rather than a handwritten guarantee about exactly what has changed. I don't think that argues against showing a semver to developers, it's just another little wrinkle to be aware of.

One important point is that I'm not opposed to npm in principle, it's just that it needs to add enough value to be worth the time it will actually take. That means looking for ways to both maximise value and minimise time.

So, what can we do to maximise value? Documentation is the thing I always value most in packages I use, but as you've no doubt guessed, I'm not that familiar with npm's particular conventions. It seems like you normally add some keywords and a small README.md with a few lines showing how to get started. Is there anything else that's generally considered valuable?

And minimising time generally means automating the release process once we've got everything set up. GitHub Actions turn out to be a huge time-saver, and I'd want to make it so pushing to GitHub automatically deployed an npm package as well. GitHub's guide for that also suggests you publish a GitHub package - I'd probably look to do that at the same time, unless there's an obvious reason not to. Also, GitHub Actions already run inside a container, so we'd probably need to build it the node way.

@andrew-sayers
Copy link
Contributor

I'm reworking the Docker build process in a way that will make this (and everything else) easier to manage. It'll take a few days, but the current plan is:

  • a new "builder" repo pushes a standard build image to Docker Hub
    • removes the need for the build step
    • gives users exactly the same build environment as GitHub Actions
  • image comes pre-installed with npm packages relevant to the build system (like jasmine) and other awkward dependencies (like libfaketime)
    • reduces the per-package build complexity
    • avoids the need to manage that weirdness in package.json
  • image comes pre-installed with standard build utilities
    • lets us write build commands that work the same in all repo's
  • as a temporary solution, the library's build script will npm install timezonecomplete etc.
    • a proper solution is a job for this issue

Example commands will look something like:

# build and test the library:
docker run --rm -it -v "/path/to/sleepdiary/library":/app sleepdiary/builder
# run the dashboard:
docker run --rm -it -v "/path/to/sleepdiary/dashboard":/app -p some_port:8080 sleepdiary/builder run

I'm planning to just push this change out and mention it here when it's done, because it seems like the sort of thing that's best reviewed in a real-world setting. If you'd like to review a PR (or rather, one PR per repository), let me know and I'll work something out.

@andrew-sayers
Copy link
Contributor

Update on how this is going:

The new build process is done. Among other things, this means people no longer need to build their own images, and commands to build a new release can go in the much smaller entrypoint.sh.

The "library" repository is now "core". Please update any links you find with the old name. The argument that finally convinced me is that we might want to add some other library some day, so it's better to avoid the generic word "library" for any particular repository.

I'm currently working on the language-agnostic parts of a release-based repository - building a changelog, PGP-signing binaries, and configuring GitHub Actions to give us a nice CI/CD pipeline. In particular, the changelog might be based on commit messages, which means I'll need to be fussier about keeping a neat commit history.

I'm increasingly convinced we should standardise on npm specifically, which means moving the dashboard away from yarn. Let me know if you want to work on that, otherwise I'll make a PR for you to look at. By the way, this is one of the reasons I didn't get back to you on the PR about adding yarn instructions - I'm pretty sure the PR won't work as written, but the exact reason changed every time I tried to write the reply.

As I mentioned elsewhere, the report and info need to be handled more cleverly than a normal npm package. They're also not yet designed for general use, and frankly I suspect they'll only ever be used in the dashboard (i.e. an already-solved problem). I don't mind adding package.json etc. to those repositories, but I'd rather release the library now and wait for a use case before worrying about the other two.

Finally, to chase up something I mentioned before - what keywords should we add to the package.json? What text would you want to see in the tarball's README.md? What else would it be valuable to put in the tarball?

@quinn-dev
Copy link
Contributor Author

I'm quite surprised you went with my original proposal of renaming it to core. I found your counterargument, that the name 'core' wrongfully implies that this library is a requirement for other libraries of the sleepdiary project, increasingly convincing. Nonetheless, I'm very happy that it now has a more distinct name.

On moving away from yarn: Is there any particular reason you want to do that? There are many reasons for and against yarn / npm, but I assume there's a particular issue with yarn that you have in mind which isn't on my radar. Furthermore, what precisely do you want to move away from yarn? The entire package.json seems to be package-manager-agnostic. You can switch from yarn.lock to package-lock.json and replace all yarn instructions in bin/entrypoint.sh with npm instructions, but I don't see the benefit really.

I agree that it currently doesn't make sense to publish info and report to npm. For the time being, core appears to be the only library with a stable enough API.

As far as keywords go, here's a (non-exhausitve) list of keywords I propose to include:

keywords: [
  'sleep',
  'diary',
  'sleep-diary',
  'sleep-format',
  'circadian',
  'circadian-rhythm',
  'sleep-tools',
  'plees-tracker',
  'sleepasandroid',
  'activitylog',
  'sleepmeter',
  'sleepchart',
  'spreadsheet'
]

The more the better, I think.

I don't think we need to add anything else other than what npm already adds by default (README, LICENSE, etc.).

andrew-sayers pushed a commit to sleepdiary/dashboard that referenced this issue Aug 1, 2021
The build script wants to download this from a public repository.
This commit can be reverted once that package goes live.

See sleepdiary/core#1
@andrew-sayers
Copy link
Contributor

Yes, like many things in this project, renaming "library" to "core" is a gamble that may or may not pay off. The way I see it, there were two obvious choices - stick with "library" or switch to "core". The former was likely to cause more problems than the latter and I couldn't think of a better option, so the best available move was to switch. But that's how development is sometimes - we made a choice and we'll deal with the consequences when the time comes.

My only strong opinion about npm vs. yarn is that we should pick one and stick with it. But during the work I've done lately on the build system (i.e. the CI/CD pipeline), I've come across projects saying "we only support npm" or "now we also support yarn", but never the other way around. For example, GitHub Packages seems to either require or strongly prefer npm, depending on who you ask. Going back to the point about gambles that might not pay off, standardising on npm seems like the move we're least likely to regret in the long-term. And yes, it would just be a matter of editing bin/entrypoint.sh and redoing the lock files, but we'd need to do that anyway for the other projects if we standardised on yarn instead.

I've now added package.json files to all the repositories. I would have made a PR, but I needed to push out at least a basic package.json to test some CI/CD stuff, so it seemed like the best move was to push a whole thing and change it based on your feedback. In particular, I've added a package-lock.json to this project in a (failed) attempt to get something to work, and will either add them to other packages or switch that one to yarn.lock depending on what we decide. I've included most of the tags you suggested, plus some Circadiaware ones, which I'll add as GitHub topics if you're ok with them.

Finally, here's a half-formed idea I'd like your feedback on:

It would be nice if we had another option for releasing code that's public but not stable, like info, report, and pre-release versions of core. For example, that would let us put them in the dashboard's package.json.

npm lets you associate a scope with a different registry such as the one GitHub Packages provides, so it wouldn't be hard to automatically publish everything to GitHub, do some live testing, then manually publish core changes to npm proper. And if someone reported a bug in core, we could push out a pre-release and get them to confirm the fix just by changing that scope's registry.

What do you think about managing packages that way?

@quinn-dev
Copy link
Contributor Author

I haven't used npm with different registries yet, but I definitely like the idea. Putting core, info and report into the dashboard's package.json was one of the reasons why I wanted things to get published to npm in the first place.

Regarding npm vs. yarn: I understand your reasons. In the end, I find yarn provides an improved development experience, but I think that's not the primary concern here.

@andrew-sayers
Copy link
Contributor

Hmm, can you give some examples of improved experience? I can't promise anything, but it's always worth knowing opportunities for improvement.

@andrew-sayers
Copy link
Contributor

Actually I don't see why a requirement to use npm necessarily implies a requirement not to use yarn. How about:

  • a repo's upgrade command must go in cmd_upgrade in entrypoint.sh
  • node repos should upgrade by doing something like npm upgrade && npm install && yarn import
  • a GitHub Action cmd_upgrades every repository once a month, and creates a PR with the relevant changes
  • cmd_test for node repos will remind you to run cmd_upgrade if it sees changes to:
    • package-lock.json but not yarn.lock
    • yarn.lock but not package-lock.json
    • package.json but not both of yarn.lock and package-lock.json
  • a GitHub Action scans every new PR for inconsistencies in those files, and...
    • if the branch is owned by sleepdiary, automatically adds a commit to the PR
    • else (e.g. if it's a PR from your fork) creates a PR to the existing PR branch

That should achieve a lot of useful goals:

  • devs can switch between npm and yarn on a whim if they so choose
  • scripts can use whichever program they were designed to work with
  • it should rarely be necessary for a human to touch the three files that need to stay in sync
  • on the rare occasion they need to edit those files, it's just one command to do everything
  • when dependabot creates PRs to upgrade packages (by creating branches in our repo), those PRs will automatically be fixed for our process
  • if dependabot misses a dependency (e.g. if cmd_upgrade includes non-npm commands), everything will still be updated regularly
  • the interface is simple enough that a new developer who doesn't understand node can produce good PRs
  • the whole system doesn't rely on humans remembering to perform vital actions

@andrew-sayers
Copy link
Contributor

Updates on this for today:

I'm going to create release notes using merge commit messages. To that end, I've made my own forks and will make changes through the normal PR process where possible. This may or may not lead to stricter policies around "good" commit messages.

Also, I should explain a side issue related to my previous post. Dependabot created a PR a few days ago that broke one of our workflows in a way that took some time to fix. In fact if you had made a PR during that time, I wouldn't have been able to accept it until the fix went in. I have now added a GitHub Action that runs unit tests on every new PR, which would have caught that bug before it was too late. That's a first step to the plan in the previous message, but I'll wait until you respond before going any further.

@andrew-sayers
Copy link
Contributor

A lot of things came together over this weekend, so this will be the first of several messages talking about how different tasks have moved forward.

I've redesigned the build process in order to support the new dev server. That involved switching everything over to npm in preparation for packages, but I'm still happy to add yarn support as discussed above. I've also added a standard set of scripts to each repo's package.json and changed the entrypoint.sh scripts to run.sh scripts that should run happily outside of a container.

I plan to wait for your input on yarn support, but my todo list is getting quite short so I may have to continue with the other parts of this issue anyway.

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

No branches or pull requests

2 participants