-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
GH-43572: [JS] Move most dependencies to the devDependencies #44517
base: main
Are you sure you want to change the base?
Conversation
|
This probably breaks the cli, no? I also think it's common to have typescript typings as dependencies. Iirc there was a similar pr recently that didn't work. |
7732ae6
to
e75311f
Compare
@domoritz Thanks for the review! I updated the PR to restore some of the imports used in the bin commands. It looks like the only js/bin being exported is the arrow2csv. But I restored the deps being imported directly in the js/bin to be safe. |
I think we need tslib and the helpers, though, no? |
Here is the pull request I was tinking of with discussion of why we need the dependencies: #43215. We could make the package lighter by removing the CLI tool from the public API. Would you like to make a pull request for that? |
"@types/command-line-args": "^5.2.3", | ||
"@types/command-line-usage": "^5.0.4", |
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.
these are indeed not needed since the cli doesn't expose types I think
"@types/node": "^20.13.0", | ||
"tslib": "^2.6.2", | ||
"@swc/helpers": "^0.5.11", |
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.
These are dependencies since they are needed at runtime or for correct types.
Rationale for this change
Adding apache-arrow as a dependency causes its dependencies to interact with the projects own dependencies. It's also bloating up JS build sizes.
Moving these dependencies to devDependencies will resolve this issue.
What changes are included in this PR?
Moves most dependencies to devDependencies in the package.json.
Are these changes tested?
n/a
Are there any user-facing changes?
This does not change user-facing APIs.