GitHub Action to deploy a release to npm

[Jolg42]

@ILOVEPIE what do you think about a GitHub Actions deployment, I can add the NPM_TOKEN there later for the npm package.

I used this recently on another project, and it could look like this maybe, any other idea is welcome 🙌🏼

Publishing a new version on npm

1. Bump the package version
   • npm --no-git-tag-version version minor (or patch for a patch release)
2. Commit files to main or merge to main via a PR
   • git add package*
   • git commit -am "Release <version>"
   • git push
3. Create a release in the UI -> https://github.com/prisma/opentype.js/releases/new
   • Add a tag, use the version, like 1.0.1
     • Click on "Create new tag: on publish"
   • Add a release title, use the version, like 1.0.1
   • Add in the description the changes and links to PRs
   • "Target" should be "main" (default)
   • "Set as the latest release" should be checked (default)
   • Click "Publish release"
   • Monitor https://github.com/prisma/opentype.js/actions/workflows/npm-publish.yml for the publish workflow

And the action

name: Publish package to npm
on:
  release:
    types: [created]
jobs:
  publish:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3

      # Setup .npmrc file to publish to npm
      - uses: actions/setup-node@v3
        with:
          node-version: '14'
          registry-url: 'https://registry.npmjs.org'

      - run: npm ci

      - run: npm run build

      # Publish to https://www.npmjs.com/package/checkpoint-client
      - run: npm publish
        env:
          NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}

[ILOVEPIE]

shouldn't we also have an action that runs dist upon a PR getting merged, also, i activated discussions so that we can keep the issues section dedicated to issues, and discussions separate.

[ILOVEPIE]

I don't mean publish on a pr getting merged, just dist, so that the repository contains the lastest build at all times.

[Jolg42]

Another option would be to get rid of dist in the Git repository, it would make things easier. Not sure if anyone is using the dist files, if someone does, we could point them to npm maybe? https://www.npmjs.com/package/opentype.js?activeTab=explore (it's a new tab where the code is available)

[ILOVEPIE]

The dist files are useful for people using non-npm based systems.

[ILOVEPIE]

plus they allow people to test the latest build from the current source code.

[ILOVEPIE]

@Jolg42 How do we go about getting this setup?

[yne]

I'm voting for the /dist/ folder removal because :

• It makes the merging process impossible since all PR conflict with each other as soon as one od them changed the .min.js version
• People who don't use npm can still fetch from the github release section, or from a NPM CDN (unpkg, cloudflare, ...)
• There is not proof that the PR don't contain a malware in the .min.js version
• Git is not meant to store build artefacts

See example at https://github.com/yne/picon/blob/master/.github/workflows/page.yml

[ILOVEPIE]

@yne fair enough, i'll do a PR that adds it to the .gitignore. My question was more on the github action for releasing to NPM & github releases.

[yne]

• Github Action shall report status on PR open/update to help reviewer (+ add built artefact to the PR?)
• Github Action shall build and publish artefact to NPM on tag creation
• /dist/ shall be purged (not added to .gitignore)

This allow the following use cases:

• If a PR fix a bug: we merge it and add an tag with an incremented micro
• If a PR add a feature: we merge it and an tag with an incremented minor

If this tagging process is correctly done, I don't see why an end-users would need an untagged version.

[ILOVEPIE]

@Connum said:

I think it's good to have the current state of the master available in the dist folder as a preview version for people who want to test it out but don't want to clone and build themselves. This might even help detecting bugs earlier.

But I also acknowledge that it's annoying to have the files cause trouble when switching branches during development. I guess the optimal way would be to have a github action build the dist files on every merge on the master branch.

Anyway, if we decide to ignore the dist files, we should also delete the ones that are already tracked before that, otherwise we'll have the outdated files in the repo forever.

I can see both sides on this one. I'm thinking maybe we have a github action that builds a new "release" (labeled something like major.minor.patch-git-commit_hash) every time a pr is merged but doesn't mark it as latest and then when we actually do tag a release it will create a release (labeled major.minor.patch) and mark it as latest. IDK, whatever you think is best.

[Connum]

If this tagging process is correctly done, I don't see why an end-users would need an untagged version.

Due to breaking changes, we're currently on our way to a major version update. It'd be great to have advanced users test it before officially releasing it.

[ILOVEPIE]

Due to breaking changes, we're currently on our way to a major version update. It'd be great to have advanced users test it before officially releasing it.

That's what release candidates are for no?

[Connum]

Sure, if those don't accidentally land on npm, that's fine. Though for me an RC is more like "almost ready, we'll only fix bugs and won't add any more feature", but we could also have -dev versions or similar.

[ILOVEPIE]

That's what I was suggesting the "releases" ending in -git-commit_hash would be.

[yne]

It'd be great to have advanced users test it before officially releasing it.

@Connum those advanced users, who are capable of debugging OpenTypeJS, are probably also capable of running an npm run build command.

If they are only supposed to do some testing, then the alpha / beta / RC lifecycle @ILOVEPIE proposed seems very reasonable.

[Connum]

Yeah, fair enough! 😄

[yne]

I made a POC on my fork that

• build + lint + test on PR or merge
• build + lint + test + publish on git tag on npm

I also took this opportunity to rework the buildflow to use esbuild / eslint / mocha.

• no more /dist folder, .travis.yml ...
• eslint warning are displayed by github !
• package.json now have only 3 devDepdendencies (and 0 vulnerabilities)
• the docs folder hold the github page (much cleaner git structure) see: demo
• the src/opentype.js work out of the box as ESModule (<script type=module>import ...), no need to bundle/build it.

Overall I'm really surprised how smooth this migration went (thanks to the clean opentypejs codebase)

@Connum what does the major upgrade brings ? could it be used to migrate this buildflow ?

[Connum]

Thanks for the work you put in! We tagged the issues and PRs we want to ship with the next version with a milestone here: https://github.com/opentypejs/opentype.js/milestone/2

If I see it correctly, you're currently targeting only modern clients with latest ES support, but I guess with esbuild it shouldn't be hard to provide a bundle for older browsers and non-module as well?

[yne]

Exactly, esbuild can output both esm and legacy (aka global) formats.

But shall we move the default entrypoint to the ESM for the 2.0.0 ?

using legacy --global-name=opentype format:

<script src="/dist/opentype.global.js"></script>
<script>
fetch('test/fonts/FiraSansMedium.woff')
.then(res => res.arrayBuffer())
.then(buf => console.log(opentype.parse(buf)))
</script>

using --format=esm we can use the import syntax (also directly work on /src/opentype.js):

<script type=module>
import * as opentype from '/dist/opentype.esm.js'; // alt: '/src/opentype.js'
fetch('test/fonts/FiraSansMedium.woff')
.then(res => res.arrayBuffer())
.then(buf => console.log(opentype.parse(buf)))
</script>

My only concern is that, to share the same ESM build across browsers and node.js, I had to strip usage of require(fs) from load() and download() functions. So the loading look like this for both platform:

// node
const buf = require('fs').readFileSync('font.woff');
opentype.parse(buf)

// browser
const buf = await fetch('font.woff').then(res => res.arrayBuffer())
opentype.parse(buf)

I can try to make load/download work, but I'm not a big fan of having platform-specific API in the codebase, especially if it only take 1 line from the caller (see: res.arrayBuffer() / fs.readFile) to do it on it side.

Again, that's a breaking change :/

[Connum]

I really wouldn't like seeing that functionality stripped. How do other libraries handle this?

[yne]

I can't find much popular hybrid file-processing oriented JS libraries. But here are some:

• mermaid 53K :star: : platform-specific are separated in a cli repo
• marked.js 29K :star: : require('fs') is limited to node-specific /bin wrapper script
• GraphicsMagick 6.8K :star: : require('fs') is limited to examples and tests. Not in src
• prettier 44.8K :star: separate node-related modules at build (I guess ?)

In the meantime I'll continue to try to make require('fs') work if ESM version is imported in a browser enviroment.

[ILOVEPIE]

@yne I think we should maintain having both an esm and a non-esm build, the current system is expected by a lot of projects, we should also maintain the same naming scheme for the files. namely opentype.js and opentype.min.js being the non-esm version and opentype.module.js being the esm version.

[yne]

@ILOVEPIE you are right. I'll keep the original naming.

[Connum]

I'm currently working on SVG-related stuff and noticed today that when we remove the dist/ folder, we'll be no longer able to use unicode-org/text-rendering-tests with pre-release versions by adding them as a github link in the package.json of the tool, but will have to replace the opentype.js file manually inside the node_modules folder. Absolute edge-case for almost anybody, but I didn't want to let it go completely unnoticed.

[yne]

@Connum I dont understand. I see in they package.json that they pull opentype.js so they will still get the dist files ...

did i missed something from they repo ?

[Connum]

It won't be a problem when it's pulled from npm, only when testing unreleased versions locally for generating reports, like I do for https://github.com/Connum/opentypejs-reports

As I said, it's not a big deal, I'll just build the files manually and copy them over.

[yne]

You could even automate this build using a github action that build opentype dist file and deploy the static website (with your built files)

[Connum]

That's a good idea!

[yne]

https://github.com/opentypejs/opentype.js/pull/567 is ready for review.

• The dist naming are kept, even if .module.js shall be .mjs to work in node (or use "type":"module" in package.json)
• .parse() accept both fs.readFile() Buffer and fetch() ArrayBuffer See loading-a-font example
• The load/loadFromUrl/loadFromFile are kept, even if they are even more useless/absurd now that it can be done in 1 line
• mocha version are untouched, since newer version require "type":"module" in package.json

So, a lot of burden that shall be lifted once we switch to 2.0.0

Any feedback are welcome

[Connum]

Great! I would keep the .load() example in the README as an addition, though, as not everyone uses modules yet and can use await in the main level of the file, which might lead to confusion.

[yne]

Understood, I'll avoid the await/async syntax. I'll also add an example to show how to load from an <input type=file>

[Connum]

You don't have to avoid it, it's good to show the way forward to modern Javascript. Just from my experience, not everyone's there yet, and there's few things more frustrating than copying code from docs just to be met with error messages! 😄 So in my opinion, we can even have the modern async syntax as the main example, and just have the older syntax for non-module usage as an addition.

[yne]

Okay, I documented both .parse() / .load() methods (with a bit of warning on the .load()) and both style (async / sync)

https://github.com/yne/opentype.yne.js#loading-a-font

[yne]

@Jolg42 You need to set the NPM_TOKEN secret to enable tag publishing.

Once set we could close this issue

[Connum]

Pinging @Jolg42 - would be great to be able to make new releases

[Jolg42]

@Connum What do you think about this?

I modified the publish script and made it more flexible, see https://github.com/opentypejs/opentype.js/commit/3cfe131c7116edaa554873d788e2b414906d572c

Here is the first run, (a dry run): https://github.com/opentypejs/opentype.js/actions/runs/6761331868

All runs will be listed at https://github.com/opentypejs/opentype.js/actions/workflows/release.yml

I already added the NPM_TOKEN, so to publish to npm, leave the dry run checkbox unchecked and that would do it 🤞🏼.

[Connum]

Great to be able to do a dry run! But I don't quite get the first version field. The second one is passed as the tag parameter to npm publish, that's clear. But why does that first one have to be specified and will be set via npm pkg set during the workflow (which will then not be reflected in the repo)? Shouldn't we set the correct version in the repos package.json before publishing anything, anyway?

[yne]

As I understand, the "version" value in git's package.json is moot. The real value is set at publish time by the CI. This way the CI is in charge of the tagging (not the package.json)

A PR can't set the version, nor multiple commit/GHaction can publish the same package.json version

Kinda agree with this workflow.

Edit: I still don't get the need of 2 variables tho (if action "version" is unset, package.json + npm publish --tag shall simply default to "latest")

[Connum]

@Jolg42 could you clarify the two version fields? I think than this would be ready to go!

[ILOVEPIE]

@Jolg42 @yne @Connum We should probably get this figgured out before the release of 2.0.0. And we want to get 2.0.0 out as fast as possible.