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

Auto test + deploy using Github Action #567

Merged
merged 9 commits into from
Feb 26, 2023
Merged

Auto test + deploy using Github Action #567

merged 9 commits into from
Feb 26, 2023

Conversation

yne
Copy link
Contributor

@yne yne commented Feb 17, 2023

Description

This PR add the following:

  • git tag creation will publish on NPM
    • Automatically change NPM package version + build + test + publish (need to setup secret)
    • Github Release page can automatically generate a release note
  • branch push or PullRequest will trigg a test + status report
    • easier/more secure merge review process
  • Github Page deployment is done by github Action
    • Action are needed to generate the assets served from the web /dist/ url
    • Move web page related assets to /docs/ folder
  • Use ESLint + ESBuild
    • More popular/maintained solution than jshint + rollup
    • NPM now report 0 vulnerabilities 🎉 (edit: NVM I had to roll back mocha, so we are back to 2 vuln)

Motivation and Context

I've made this PR because it's a burden to

  • Accept PR that possibly contain hidden malware in they minified dist files => /dist/ files are built on CI + published.
  • Manually do the versioning in the package.json (only @Jolg42 can publish on NPM anyway) => any maintainer can now do it from the gituhub release page.
  • Update the RELEASE.md => redirect to the Github Release page that contain auto-generated summaries
  • Update the CONTRIBUTOR.md => redirect to the Github Contributor page that contain all contributor
  • See a vulnerabilities warning each time I install opentype.js

How Has This Been Tested?

I've forked the opentype.js repos as opentype.yne.js and deployed it on NPM and on github page

I've also tested the following developer use cases (after npm i opentype.yne.js) :

index.mjs

import opentype from 'opentype.yne.js';
import { parse } from 'opentype.yne.js';

// the folowing import don't work:
// import mod from 'opentype.yne.js/dist/opentype.module/js';
// Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.

console.log(opentype, parse)

index.js

const ot = require('opentype.yne.js');
console.log(ot)
<!-- same as https://unpkg.com/opentype.yne.js -->
<script src="https://unpkg.com/opentype.yne.js/dist/opentype.js"></script>
<script>console.log(opentype.parse)</script>

<script type=module>
import { parse } from "https://unpkg.com/opentype.yne.js/dist/opentype.module.js"
console.log(parse);
</script>

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I did npm run test and all tests passed green (including code styling checks).
  • I have added tests to cover my changes.
  • My change requires a change to the documentation.
  • I have updated the README accordingly.

src/util.js Show resolved Hide resolved
@yne yne force-pushed the master branch 4 times, most recently from cc21f59 to 5b2bf01 Compare February 19, 2023 02:03
@yne yne marked this pull request as ready for review February 19, 2023 02:17
@yne
Copy link
Contributor Author

yne commented Feb 19, 2023

All API and naming are keept for backward compatibility.
If not, warn me.

Copy link
Contributor

@ILOVEPIE ILOVEPIE left a comment

Choose a reason for hiding this comment

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

This should probably be broken up into smaller PRs, there's also some changes here that were not properly thought through. Honestly this would have been a lot easier to review if it was two PRs, a build system update and one that adds the github actions.

.editorconfig Show resolved Hide resolved
.github/workflows/page.yml Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
docs/glyph-inspector.html Outdated Show resolved Hide resolved
docs/index.html Outdated Show resolved Hide resolved
test/tables/ltag.js Show resolved Hide resolved
test/tables/meta.js Show resolved Hide resolved
test/tables/name.js Show resolved Hide resolved
test/tokenizer.js Show resolved Hide resolved
test/types.js Show resolved Hide resolved
@yne
Copy link
Contributor Author

yne commented Feb 21, 2023

This should probably be broken up into smaller PRs

Sorry about that, I wanted the GithubActions to run without vulnerabilities, which lead me to updating our buildtools/testtool, which lead me to fix many lint warning etc...

This is why I carefully split my commit by subjects (action, lint, ...) : to ease the reading (I hope you didn't read this PR as a single page ...).

Also avoid repeating the same comment:

  • It's a pain for you, who did a lot of work.
  • It's a pain for me, who have to focus on the important one, and close all duplicate after the first explanation.
  • It's a pain for others reviewer who need to browse the ~6 subject you raised.

I hope I answered all your question and fix all the changed you asked.
I you spot some remaining mistakes, let me know 👍

I think I'll add a jsdoc enforcement since some prototype (.load()) dont match between jsdoc and extern/opentype.js

@ILOVEPIE
Copy link
Contributor

Yeah sorry about the duplicated messages, I was trying an integrated PR reviewing tool in my code editor, I guess it applies the message to everywhere it finds a match if you use it during a code-search.

@yne
Copy link
Contributor Author

yne commented Feb 23, 2023

rebased over #566 changes

@ILOVEPIE @Connum do you want/need any other changes ?

Copy link
Contributor

@ILOVEPIE ILOVEPIE left a comment

Choose a reason for hiding this comment

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

Just need a little clarification on some stuff and one small change to package.json

package.json Show resolved Hide resolved
src/tiny-inflate@1.0.3.esm.js Show resolved Hide resolved
Copy link
Contributor

@ILOVEPIE ILOVEPIE left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@Connum
Copy link
Contributor

Connum commented Feb 23, 2023

Before someone merges this, give me a second please, I want to add some things!

@yne
Copy link
Contributor Author

yne commented Feb 23, 2023

@Connum no problem, I'll let you do this merging when you are ready/done 👍

@Jolg42 You'll have to setup a Github Action "Repository secrets" named NPM_TOKEN to make the npm publish work

Copy link
Contributor

@Connum Connum left a comment

Choose a reason for hiding this comment

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

I didn't have the time for a thorough test, working on too many other things... But I did quickly check this out and try it. Here is something I stumbled upon:

  1. I was a bit confused when running npm run start and instead of getting the dev server with the index.html, which I frequently use during development
  2. So, I added the parameter --servedir=\"./\" to the start script, then we can open the pages at http://127.0.0.1:8000/docs/ (we should update the "Contribute" section in the README to lead new contributors there)
  3. However, we have to change the paths to the js file in the html files as well, src="dist/opentype.js" to src="../dist/opentype.js" (plus one level down for the files in /docs/examples). And we also need to add the parameter --global-name=opentype to the script in order to have the opentype global available.
  4. Unfortunately, esbuild doesn't support HMR (see serve dir evanw/esbuild#796) ☹ But at least we can add the parameters --watch --footer:js=\"window.IS_DEV_BUILD=true\" parameter and add this to the html files:
if(window.IS_DEV_BUILD) {
    new EventSource('/esbuild').addEventListener('change', () => location.reload());
}

Now, the page will reload on file changes, which is good enough for me now. So, the final start script should look like this:

"start": "esbuild --bundle src/opentype.js --outdir=dist --external:fs --target=es2018 --watch --serve --servedir=\"./\"",

If you prefer, I can do those changes and add them to the PR, just let me know.

We must also not forget to set the GitHub pages root to the /docs folder!

But I have to say, I love how blazing fast the build process of esbuild is compared to even vite, which I mainly use for my projects right now. That's really great!

@Connum Connum added the dev Changes revolving merely around dev-related code like testing, build process, etc. label Feb 23, 2023
@Connum
Copy link
Contributor

Connum commented Feb 23, 2023

Another thing: I'm not able to run npm run test, getting SyntaxError: Unexpected end of input. Trying to figure out why, I noticed that indeed the code that is added via the esbuild parameter "--footer:js" is truncated after the first linebreak.
This might be a Windows-related issue, but unlesse you want me to stop contributing, we should find a solution for that! 😅

Edit: And I think the test command should run the build and dist before running mocha, so we always have all the latest files.

@Connum
Copy link
Contributor

Connum commented Feb 24, 2023

I can't find a way to make an npm script (test) depend on another npm script (dist)

Shouldn't "test": "npm run build && npm run ist && mocha --require reify --recursive", work?
And in the current setup we have the linting run automatically after testing, which could otherwise be forgotten if you have to call it manually, so adding && npm run lint could be a good idea as well?

@yne
Copy link
Contributor Author

yne commented Feb 24, 2023

I can't find a way to make an npm script (test) depend on another npm script (dist)

Shouldn't "test": "npm run build && npm run ist && mocha --require reify --recursive", work? And in the current setup we have the linting run automatically after testing, which could otherwise be forgotten if you have to call it manually, so adding && npm run lint could be a good idea as well?

Good point, I'll change that 👍

@yne
Copy link
Contributor Author

yne commented Feb 24, 2023

rebased over #575 and over #572
@Connum I updated the code according to the discussion

  • npm run test now also build dev + prod dist files
  • npm run start now serve the global file + auto-reload + use /dist/... as script src
  • npm run lint now fail upon fixable errors
  • README.md was edited to reflect those changes

Tell me if I missed anything

package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@Connum Connum left a comment

Choose a reason for hiding this comment

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

For some reason, the page reload on rebuild does not get triggered...
And I have to double-escape all line breaks in the script parameters to \\n in order for the scripts to work.
We can fix that later in order to get this finally over with, but could you please check if the commands still work for you with double-encoded line breaks?

@yne yne merged commit f3baebc into opentypejs:master Feb 26, 2023
@yne
Copy link
Contributor Author

yne commented Feb 26, 2023

@Connum

For some reason, the page reload on rebuild does not get triggered...

Fixed, see #578

And I have to double-escape all line breaks in the script parameters to \\n in order for the scripts to work. We can fix that later in order to get this finally over with, but could you please check if the commands still work for you with double-encoded line breaks?

\\n generate a SyntaxError: Invalid or unexpected token on Linux (because it output a literal \n )

I can remove all \n if it breaks windows build, or invert " and ' for those actions, for example:

{
    "start": "esbuild --bundle src/opentype.js --outdir=dist --external:fs --target=es2018 --format=iife --out-extension:.js=.js      --global-name=opentype --footer:js='(function (root, factory) {\nif (typeof define === \"function\" && define.amd)define(factory);\nelse if (typeof module === \"object\" && module.exports)module.exports = factory();\nelse root.opentype = factory();\n}(typeof self !== \"undefined\" ? self : this, () => ({...opentype, \"default\" : opentype })));' --watch --servedir=. --footer:js=\"new EventSource('/esbuild').addEventListener('change', () => location.reload())\"",
    "b:umd": "esbuild --bundle src/opentype.js --outdir=dist --external:fs --target=es2018 --format=iife --out-extension:.js=.js      --global-name=opentype --footer:js='(function (root, factory) {\nif (typeof define === \"function\" && define.amd)define(factory);\nelse if (typeof module === \"object\" && module.exports)module.exports = factory();\nelse root.opentype = factory();\n}(typeof self !== \"undefined\" ? self : this, () => ({...opentype, \"default\" : opentype })));'",
    "d:umd": "esbuild --bundle src/opentype.js --outdir=dist --external:fs --target=es2018 --format=iife --out-extension:.js=.min.js  --global-name=opentype --footer:js='(function (root, factory) {\nif (typeof define === \"function\" && define.amd)define(factory);\nelse if (typeof module === \"object\" && module.exports)module.exports = factory();\nelse root.opentype = factory();\n}(typeof self !== \"undefined\" ? self : this, () => ({...opentype, \"default\" : opentype })));' --minify --sourcemap",
}

I'll let you try and tell me

@Connum
Copy link
Contributor

Connum commented Feb 26, 2023

I just noticed that the font inspector and glyph inspector don't work, while the index page does: https://opentype.js.org/index.html
I'm quite sure it all worked when I tested it locally while reviewing, but I'm not able to test that right now

@yne
Copy link
Contributor Author

yne commented Feb 26, 2023

The pages are working, but they "demo" font is not loaded (404)

  • index load FiraSansMedium.woff by default
  • while other pages load Roboto-Black.ttf by default

I'll unify them all to FiraSansMedium.woff

Hotfixed : c75791a

@Connum
Copy link
Contributor

Connum commented Feb 27, 2023

{
    "start": "esbuild --bundle src/opentype.js --outdir=dist --external:fs --target=es2018 --format=iife --out-extension:.js=.js      --global-name=opentype --footer:js='(function (root, factory) {\nif (typeof define === \"function\" && define.amd)define(factory);\nelse if (typeof module === \"object\" && module.exports)module.exports = factory();\nelse root.opentype = factory();\n}(typeof self !== \"undefined\" ? self : this, () => ({...opentype, \"default\" : opentype })));' --watch --servedir=. --footer:js=\"new EventSource('/esbuild').addEventListener('change', () => location.reload())\"",
    "b:umd": "esbuild --bundle src/opentype.js --outdir=dist --external:fs --target=es2018 --format=iife --out-extension:.js=.js      --global-name=opentype --footer:js='(function (root, factory) {\nif (typeof define === \"function\" && define.amd)define(factory);\nelse if (typeof module === \"object\" && module.exports)module.exports = factory();\nelse root.opentype = factory();\n}(typeof self !== \"undefined\" ? self : this, () => ({...opentype, \"default\" : opentype })));'",
    "d:umd": "esbuild --bundle src/opentype.js --outdir=dist --external:fs --target=es2018 --format=iife --out-extension:.js=.min.js  --global-name=opentype --footer:js='(function (root, factory) {\nif (typeof define === \"function\" && define.amd)define(factory);\nelse if (typeof module === \"object\" && module.exports)module.exports = factory();\nelse root.opentype = factory();\n}(typeof self !== \"undefined\" ? self : this, () => ({...opentype, \"default\" : opentype })));' --minify --sourcemap",
}

I'll let you try and tell me

With the quotes stopped, the commands fail completely, so I guess we'll have to get rid of the line breaks!

@yne
Copy link
Contributor Author

yne commented Feb 27, 2023

we'll have to get rid of the line breaks!

done: 234a535

@Connum
Copy link
Contributor

Connum commented Feb 27, 2023

Great! Can you add it to master?

@ILOVEPIE
Copy link
Contributor

Sorry been out of the loop a little bit as i've been a bit busy. I'll spend tommorow trying to catch up.

@yne
Copy link
Contributor Author

yne commented Feb 28, 2023

Great! Can you add it to master?

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev Changes revolving merely around dev-related code like testing, build process, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants