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

Sane buildflow #703

Merged
merged 4 commits into from
May 24, 2024
Merged

Sane buildflow #703

merged 4 commits into from
May 24, 2024

Conversation

yne
Copy link
Contributor

@yne yne commented Apr 21, 2024

Description

Based on existing work done by @ILOVEPIE in #693 this PR migrate to a sane build pipeline (which is urgent) while leaving any compatibility-related change to #693 (which is nice, but less urgent).

  • Rename src/*.js to src/*.mjs and test/*.js to test/*.spec.mjs
  • Remove unused reify dependency
  • Use latest Mocha => no more critical vulnerabilities when npm i
  • Use latest ESLint 9 => need to migrate to they new config format
  • Deprecate load*/download functions (Why did we depricate the load function? #675) and remove associated tests
  • Remove dead code (private load*() function, isNode/isBrowser...)
  • Fix some issues in recent tests addition
  • Add support for many popular test runner:
npx mocha
npx bun test
npx jasmine "**/*.spec.mjs" # may fail on strict compare

BONUS: This PR also cleanup our ESLint rules:

  • "ban forEach" is migrate to a "selector" rule
  • prevent import loop => did nothing
  • force import file extension => esbuild enforce it already

Tip

This halves (243 => 144) our NPM dependencies

Motivation and Context

Lower our attack surface by half, while keeping the backward-compatibility improvment in #693

How Has This Been Tested?

npm i && npm run test

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.
  • I have read the CONTRIBUTING document.

@yne yne requested review from ILOVEPIE and Connum April 21, 2024 13:02
yne added 2 commits April 21, 2024 15:02
- Rename src/*.js to src/*.mjs and test/*.js to test/*.spec.mjs
- Move misplaced "eslint-plugin-local-rules" to devDependencies
- Remove unused reify now that we are fully .mjs
- Deprecate load*/download functions and remove associated tests
- Remove dead code (private load*() function, isNode/isBrowser...)
- Support multiple test runner:

```sh
npx mocha
npx bun test
npx jasmine "**/*.spec.mjs" # may fail on strict compare
```
There are currently 3 local rules:
- ban forEach => migrate to a "selector" rule
- prevent import loop => did nothing
- force import extension => esbuild enforce it already

This halves (226 => 139) our NPM dependencies
@yne yne force-pushed the sane-buildflow branch from 169ca7e to 48ea8a8 Compare April 21, 2024 13:02
Use new configuration format since package.json based configuration have been droped
@yne yne force-pushed the sane-buildflow branch from 8ccc0dc to e26c62c Compare April 21, 2024 14:04
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.

See my comments - other than that, this looks good! Nice work getting rid of all those depedencies and moving towrds a more modern code base.

src/glyphset.mjs Show resolved Hide resolved
src/tables/cpal.mjs Show resolved Hide resolved
src/tables/glyf.mjs Show resolved Hide resolved
src/font.mjs Show resolved Hide resolved
test/bidi.spec.mjs Show resolved Hide resolved
test/glyph.spec.mjs Show resolved Hide resolved
test/glyph.spec.mjs Show resolved Hide resolved
test/glyph.spec.mjs Show resolved Hide resolved
src/util.mjs Show resolved Hide resolved
test/util.js Show resolved Hide resolved
@yne
Copy link
Contributor Author

yne commented Apr 21, 2024

Nice work getting rid of all those depedencies and moving towrds a more modern code base.

Props to @ILOVEPIE who did the initial migration. I just reused it PR but stopped before doing legacy-compatibility stuff so he could rebase it PR on it

@ILOVEPIE
Copy link
Contributor

So, the point of this is to split my pr into two?

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.

I can't approve of this in it's current state.

eslint-local-rules.js Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
src/opentype.mjs Show resolved Hide resolved
src/opentype.mjs Show resolved Hide resolved
src/opentype.mjs Show resolved Hide resolved
test/opentype.spec.mjs Show resolved Hide resolved
test/opentype.spec.mjs Show resolved Hide resolved
test/opentype.spec.mjs Outdated Show resolved Hide resolved
@Connum
Copy link
Contributor

Connum commented Apr 22, 2024

As my brain needed a break from trying to implement CFF2 writing for variable glyphs, I'm currently working on a slim (=really just a couple of lines) plugin API to offer support for standalone CFF1 and PS1/Adobe Type 1 files without bloating the main library.

I think I would be on board with removing the load and download functions directly in 2.0.0 without prior depreciation notice if we were to offer a "browserhelper" and "nodehelper" plugin at the same time, references in a console message when trying to call the functions. The browserhelper could also provide the functionality for adding a preview of the font via CSS @font-face as discussed in #62.

That way, all that users had to do would be to add or import another small file delivered together with our lib in order to keep the functionality.

What do you guys think?

@yne
Copy link
Contributor Author

yne commented Apr 22, 2024

What do you guys think?

for the load/download function, a simple line in the "usage" chapter of the readme would suffice as it is just demonstration of how user can integrate opentype.js in they environment.

but I agree for a separation of concern

This is what jasmine is doing (jasmine/jasmine-core)

@yne
Copy link
Contributor Author

yne commented Apr 22, 2024

So, the point of this is to split my pr into two?

Yes

  • this PR remove reify so we can bump Mocha
  • Your PR integrate SWC to add a .legacy.js build

@Connum @ILOVEPIE : I closed dups comment to keep a readable thread

@yne yne force-pushed the sane-buildflow branch from 2baa5e6 to 38abe35 Compare April 22, 2024 19:15
@ILOVEPIE ILOVEPIE self-requested a review April 24, 2024 19:00
@ILOVEPIE
Copy link
Contributor

ILOVEPIE commented May 9, 2024

Hey, @yne can we get this rebased on the current head so that it can be reviewed and merged if it's ok?

@yne
Copy link
Contributor Author

yne commented May 9, 2024

Hey, @yne can we get this rebased on the current head so that it can be reviewed and merged if it's ok?

sure, I'll ping you once I get back home (~ end of week)

@yne
Copy link
Contributor Author

yne commented May 14, 2024

@ILOVEPIE It is in fact, already rebased on master.

So there was not much to do

So instead I compiled opentype.mjs as binary using QuickJS and the following bin/qjs.mjs CLI frontend:

import { parse } from "../src/opentype.mjs";
import * as std from 'std';
const [_, path = null, opt = '{lowMemory:false}'] = scriptArgs;
function readFile(path) {
    const rh = std.open(path, "rb");
    rh.seek(0, std.SEEK_END);
    const sz = rh.tell();
    const ab = new ArrayBuffer(sz);
    rh.seek();
    rh.read(ab, 0, sz);
    rh.close();
    return ab;
}

try {
    if (!path) throw ("USAGE:\n\topentype FILE [OPT_JSON]");
    const font = parse(readFile(path), std.parseExtJSON(opt));
    console.log(JSON.stringify(font.tables, null, '\t'));
} catch (e) {
    console.log(e.stack || e)
}

Then strip-compiled it with:

qjsc -flto bin/qjs.mjs -o dist/opentype && strip dist/opentype

Now I can parse our 24 OTF test file and get they fontFamily in 1 second

time for ttf in test/fonts/*.ttf ; do dist/opentype $ttf ; done | jq .name.windows.fontFamily.en

"Changa"
"Changa ExtraLight"
...
"VARTest VF"
"Zycon"

real	0m1,171s
user	0m1,058s
sys	0m0,112s

Here is the binary opentype.linux_x64.gz (I had to add a fake .gz extension to bypass the upload file whitelist)

maybe we could distribute opentype.js as standalone binary (but I don't see much use case except dumping the OTF as JSON for JQ post-processing)

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.

@yne yne requested a review from Connum May 19, 2024 06:47
@yne
Copy link
Contributor Author

yne commented May 22, 2024

We can merge if @Connum is satisfied with the current justifications

@Connum Connum merged commit 68c43ac into master May 24, 2024
1 check passed
@Connum Connum deleted the sane-buildflow branch May 24, 2024 09:28
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

Successfully merging this pull request may close these issues.

3 participants