Skip to content

Conversation

@jacobtread
Copy link

@jacobtread jacobtread commented Apr 28, 2025

Before submitting the PR, please make sure you do the following

Bumps esbuild to the latest version to fix the vulnerability warning

Closes #264

  • It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.
  • This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
  • Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Cannot run "npm run lint" as I don't have the right typescript version (What I understand from the error message) so it just crashes. But changes are minor enough that I don't think I could have failed any lints unless they were already failing.

  • Edit looks like linting is passing in CI so all good there

Tests

  • Run the tests tests with npm test or yarn test

Fixed up one of the tests in utils.test.ts which was failing from a type error. Other than that all tests are passing

"cli-color": "^2.0.3",
"deepmerge": "^4.2.2",
"esbuild": "^0.19.2",
"esbuild": "^0.25.3",

Choose a reason for hiding this comment

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

can you also move it to devDependencies? that will avoid this from being a problem in the future and avoid people from downloading it in the first place

Copy link
Author

@jacobtread jacobtread May 24, 2025

Choose a reason for hiding this comment

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

If the maintainer expresses interest in merging it then I'll make any additional changes but if it's not going to get merged I'll hold off on any continued changes 😅

Choose a reason for hiding this comment

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

well if you have to wait a couple of months for him to come tell you that he wants it changed and then you have to wait a couple of months more for the actual merge then you're just prolonging things. seems better to do it now and allow it to be merged more quickly when it's gotten around to

Choose a reason for hiding this comment

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

I've sent an improved fix if you'd like to close this one: #270

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.

Dependency update (esbuild)

2 participants