-
-
Notifications
You must be signed in to change notification settings - Fork 425
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
Remove dead dependency snake-case #938
base: main
Are you sure you want to change the base?
Remove dead dependency snake-case #938
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -40,7 +40,7 @@ | |||
"commander": "^9.4.1", | |||
"dashify": "^2.0.0", | |||
"glob": "^8.0.3", | |||
"snake-case": "^3.0.4" | |||
"just-snake-case": "^3.2.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not change-case instead?
- It could also replace
camelcase
change-case
is smaller thancamelcase
+just-snake-case
zipped- it's also fewer files/1 fewer dependency
- It's what the author recommends (same author)
- It's more popular (projects will more likely have
change-case
thanjust-snake-case
already in theirnode_modules
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could switch to change-case, but version 5.x is ESM-only, and I believe only support node 16.x and up.
Version 4.x of change-case also works, but I'm not sure that one is as small as the 5.x branch (it is at least still structured as a cluster of several sub-dependencies in v4.x).
You could argue that is still a better fit than changing to just-snake-case, though, in which case I'm happy to open a PR that replaces camelcase+snake-case to change-case 4.x!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Node 16 (which is long dead btw!) does support ESM. See
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, supporting only node 16 from now on would be good.
I think the issue I'm seeing is that this project still supports node 14 and still builds for commonjs:
Command failed: /Users/stiaje/Projects/svgr/packages/cli/bin/svgr --no-index __fixtures__/simple --out-dir=__fixtures_build__/no-index-case
/Users/stiaje/Projects/svgr/packages/core/dist/index.js:4
var changeCase = require('change-case');
^
Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/stiaje/Projects/svgr/node_modules/.pnpm/[email protected]/node_modules/change-case/dist/index.js from /Users/stiaje/Projects/svgr/packages/core/dist/index.js not supported.
Instead change the require of /Users/stiaje/Projects/svgr/node_modules/.pnpm/[email protected]/node_modules/change-case/dist/index.js in /Users/stiaje/Projects/svgr/packages/core/dist/index.js to a dynamic import() which is available in all CommonJS modules.
If I'm just missing something stupid to be able to have jest also run on ESM code, though, that's great!
Summary
resolves #937
snake-case
is a deprecated repo.just-snake-case
does the same thing with fewer dependencies.Test plan