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

Upgrade to Node 18. #24

Merged
merged 10 commits into from
Jan 9, 2023
Merged

Upgrade to Node 18. #24

merged 10 commits into from
Jan 9, 2023

Conversation

ray-lee
Copy link
Contributor

@ray-lee ray-lee commented Dec 16, 2022

Description

Upgrades this project to build with Node 18, along with some other quality/security improvements. This includes:

  • Upgrading dependencies that were incompatible with Node 18
  • Upgrading dependencies to resolve npm audit security errors (not all of them, just the easy ones for now)
  • Code/configuration changes required by major version upgrades of dependencies, including
    • Major typescript update required adding some missing type parameters to Promise constructors
    • Major webpack update required updating some webpack config files, moving some npm script arguments into webpack config files, and removing some no-longer-supported arguments
  • Some minor formatting changes to resolve lint errors
  • Additional exports from some files to resolve documentation generation warnings

Motivation and Context

The project had been using Node 12, which is no longer supported, and will not be allowed in GitHub actions soon. Notion: https://www.notion.so/lyrasis/Upgrade-web-opds-client-to-Node-18-44f9a03cbb0545cc863f19a5db8bb9c0

How Has This Been Tested?

  • Verify that npm install runs without error
  • Verify that all npm scripts in all package.json files (package.json, packages/server/package.json, and packages/web-opds-client/package.json) run without error (except for npm publish, which we'll test when we actually publish). This especially includes npm test in packages/web-opds-client.
  • Verify that the app runs in dev:

Checklist:

  • I have updated the documentation accordingly.
  • All new and existing tests passed.

@ray-lee ray-lee marked this pull request as ready for review December 16, 2022 23:30
@ray-lee ray-lee requested a review from a team December 16, 2022 23:30
Copy link

@tdilauro tdilauro left a comment

Choose a reason for hiding this comment

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

This looks good and seems to work fine.

I do have one question about the husky dependency being reverted to an older version. Could you clarify?

package.json Show resolved Hide resolved
@@ -32,8 +36,7 @@
},
"lint-staged": {
"*.{js,jsx,ts,tsx,json,md}": [
"prettier --write --ignore-path .gitignore",
"git add"

Choose a reason for hiding this comment

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

Just adding a not of explanation here for future reference that with lint-staged v10 and above, the "git add" can be omitted. From the docs:

From v10.0.0 onwards any new modifications to originally staged files will be automatically added to the commit. If your task previously contained a git add step, please remove this. The automatic behaviour ensures there are less race-conditions, since trying to run multiple git operations at the same time usually results in an error.

@@ -14,6 +14,7 @@
},
"exclude": ["node_modules", "lib", "dist"],
"typedocOptions": {
"entryPointStrategy": "expand",

Choose a reason for hiding this comment

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

Explanation: "expand" behavior was the default before v0.22. For v0.22+, "resolve" behavior is the default. We're updating from v0.20... to v0.23..., so this change is to retain the "expand" behavior.

From the docs:

The default behavior in v0.21 and earlier. Behaves like the resolve behavior, but will recursively expand directories into an entry point for each file within the directory.

@ray-lee
Copy link
Contributor Author

ray-lee commented Jan 4, 2023

@tdilauro Could you approve this? I think everything has been resolved.

Copy link

@tdilauro tdilauro 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! 🚀

@ray-lee ray-lee merged commit 0b8078f into main Jan 9, 2023
@ray-lee ray-lee deleted the upgrade-node-18 branch January 9, 2023 18:34
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.

2 participants