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

Fix build, add lock file, upgrade GitHub Actions #218

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

Conversation

thomastoye
Copy link

@thomastoye thomastoye commented Sep 2, 2022

Hi @epoberezkin, I've fixed the build, upgraded GitHub Actions to the latest version, dropped old Node versions and added 16.x and 18.x, and added a lock file to prevent the build from breaking due to in-range update breakage.

Let me know if this looks good. I'd like to add ESM output in a subsequent PR.

@thomastoye thomastoye changed the title Fix build, add lock file Fix build, add lock file, upgrade GitHub Actions Sep 2, 2022
Copy link
Contributor

@G-Rath G-Rath 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 - fwiw I think it's probably worth just typing the errors as any for now rather than casting; ideally we should also have an .npmignore that excludes the package-lock.json

@epoberezkin
Copy link
Member

I think recommendation is to use lock files only in applications, but not in the libraries.

@G-Rath
Copy link
Contributor

G-Rath commented Apr 21, 2023

The general opinion is pretty divided - fwiw I say you should always commit your lockfiles, because you can always have CI workflows for libraries that remove them in order to test that everything works with the latest versions while still retaining the lockfile to prevent issues like this from breaking the general development flow.

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

Successfully merging this pull request may close these issues.

None yet

4 participants