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

build: upgrade all the things #67

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

Robbert
Copy link

@Robbert Robbert commented Oct 19, 2024

Oops, I accidentally already created a PR.

To do:

  • Discuss: fix eslint no-useless-escape issues removes regexp escapes. However the existing ESLint config specifically disables this rule. Is there a reason the escapes are actually needed? In that case:
    • revert my commit
    • disable no-useless-escape in the new eslint config
  • move codecov SaaS to reparate PR
  • fix errors
  • discuss: fetch is now available in Node.js, and the shim-fetch.js from lib-font is no longer being used. This causes all tests to fail, because local paths are treated differently. I'm not sure if this is a breaking change, or if the fromPath function is already broken in Node 22 in the current release.
    • Update: I investigated the use of fromPath and fromDataBuffer. fromPath is uses in Node.js only, and fromDataBuffer is used in the browser mainly. Making fromPath Node.js only makes sense to me. For browser support I suggest adding a new fromURL API.

@Robbert Robbert marked this pull request as draft October 19, 2024 10:58
@Robbert Robbert force-pushed the build/updates branch 2 times, most recently from 26c6d6d to e0289b5 Compare October 19, 2024 11:54
@Robbert Robbert marked this pull request as ready for review October 19, 2024 11:54
@RoelN
Copy link
Contributor

RoelN commented Nov 1, 2024

Thanks Robbert! 🙏

I think there's something wrong with the linter/prettier, as running npm run prettier makes changes we don't want, like " → '. Likewise for npm run lint-fix.

Also npm run lint gives ERROR: "lint:prettier" exited with 1. 🤔

The rest of the changes look reasonable to me, but I'd love an extra pair of eyes on this. Maybe @Ryuno-Ki would like to do the honors?

Copy link

@Ryuno-Ki Ryuno-Ki 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, by and large.

node-version: ${{ matrix.node-version }}
- run: npm install
node-version-file: .nvmrc
- run: npm ci
Copy link

Choose a reason for hiding this comment

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

This works.

If you want to disable the postinstall scripts, npm ci --ignore-scripts would be the option to set.
This might prevent malicious scripts from being executed.


export default [
{
languageOptions: { globals: globals.browser },
Copy link

Choose a reason for hiding this comment

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

I usually see this option at the last position (so it can override previous ones).

I guess giving this configuration it should work ™️ , though.

import Fondue from "./src/fondue/Fondue.mjs";

export async function fromPath(fontPath) {
const { readFile } = await import("node:fs/promises");
Copy link

Choose a reason for hiding this comment

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

Curious why this wasn't imported in line 16, but that shouldn't block the PR.

"prettier": "^2.0.5",
"rollup": "^2.21.0"
},
"jest": {
Copy link

Choose a reason for hiding this comment

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

Where did this configuration move to?
(Now vitest is used over jest)

"@rollup/plugin-json": "6.1.0",
"@rollup/plugin-node-resolve": "15.3.0",
"@vitest/coverage-v8": "2.1.3",
"babel-jest": "29.7.0",
Copy link

Choose a reason for hiding this comment

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

Is this still needed given vitest?

"collectCoverageFrom": [
"src/*.{js,jsx}"
]
"@babel/preset-env": "7.25.8",
Copy link

Choose a reason for hiding this comment

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

If babel-jest is dropped, can this be removed as well?

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