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

SNOW-1650373: Too many dependencies #906

Closed
DScheglov opened this issue Sep 4, 2024 · 4 comments
Closed

SNOW-1650373: Too many dependencies #906

DScheglov opened this issue Sep 4, 2024 · 4 comments
Assignees
Labels
duplicate This issue or pull request already exists status-triage_done Initial triage done, will be further handled by the driver team

Comments

@DScheglov
Copy link

DScheglov commented Sep 4, 2024

What is the current behavior?

Installing the snowflake-sdk on the empty project brings ~305 unique dependencies.
Installing it to our existing project +160 (plus around 15% to what we have now).

Do we realy need smithy, aws/s3-client, moment!!, axios!! In the driver???

What is the desired behavior?

It is better to minimize 3rd party dependencies to less then 10.
Ideally it must not have any 3rd party dependency

How would this improve snowflake-connector-nodejs?

Currently we have to consider using another DB, because Snowflake doesn't have
a trustable (I really cannot understand why I need smithy in the driver) nodejs-driver.

References, Other Background

Please consider this comparision:

https://gist.github.com/DScheglov/edfc385c4467abaf300ca56f06a8aee3

@github-actions github-actions bot changed the title Too many dependencies SNOW-1650373: Too many dependencies Sep 4, 2024
@sfc-gh-dszmolka sfc-gh-dszmolka self-assigned this Sep 4, 2024
@sfc-gh-dszmolka sfc-gh-dszmolka added duplicate This issue or pull request already exists status-triage_done Initial triage done, will be further handled by the driver team labels Sep 4, 2024
@sfc-gh-dszmolka
Copy link
Collaborator

we're tracking the request of removing cloud dependencies at #449

axios will stay for a while, being the main workhorse for http communication for the moment.

closing it as a duplicate.

@DScheglov
Copy link
Author

DScheglov commented Sep 4, 2024

@sfc-gh-dszmolka , with all respect.

Are moment and moment-timezone cloud-specific dependenecies?

Or fast-xml-parser is?

We already have xml2js, why do we need a new one? Actually, xml-parser is not an easy to be added to the project, beause it is a potential source of vulnarabilities (more then other npm packages). And actually we don't have an xml fields, so why do we need this dependency at all? (we need xml2js to work with SAML).

Or winston? Why does driver install winston? And yes, is winston also a cloud-specific dependency?

It seems the driver does a lot of work on behalf of its clients from one side and has a ... low level API from other side. Perhaps it makes sense to change the balance on the counter side: make convinient API and make customization through the injection, doesn't it?

So, I don't consider this issue as a duplicated for cloud-specific one.

And yes, unfortunatelly we cannot consider this driver until it depends on axios and moment (because it will be another package when this dependencies will be removed, so it will require to be reviewed again before including to the project).

@sfc-gh-dszmolka
Copy link
Collaborator

sfc-gh-dszmolka commented Sep 4, 2024

thank you @DScheglov for taking the time to describe your concern in detail. While one could agree with all of them, you also need to consider that certain parts of the library come from 6 years ago and well, the industry and best practices changed in the meantime so you're right to expect that every library follows the same trends and the library is constantly refactored and optimized.
Or even the whole approach, which worked 6 years ago, reconsidered and reimplemented, maybe with 10 dependencies which better fits today's trends. Maybe Promises instead of callbacks, and so on. There are a lot of elements to consider and refactor.

It is always possible to improve the driver, and we always need to decide how we allocate the resources who needs to be divided between your request and all the others. This is of course not your problem. That's why I ask you to kindly contact your Snowflake account representative and explain to them how this request is important (or even a blocker!) to your use case. They can then directly work with the product team to help prioritize amongst all the other requests.

Alternatively if you have time and possibility, you can also submit PR which then could be reviewed by the team and we thank you for your contribution in advance!

Even more alternatively, perhaps you could also take a look at Snowflake's SQL REST API - depending on your use-case, maybe it even better fits your requirements as you can directly talk to Snowflake without a driver, with a http library of your choice without any other dependencies. (at the cost of some capabilities, which you might or might not even use)

@DScheglov
Copy link
Author

DScheglov commented Sep 4, 2024

@sfc-gh-dszmolka

is this driver just a wrapper around this SQL REST API? Or it also supports other interfaces?

That's why I ask you to kindly contact your Snowflake account representative and explain to them how this request is important (or even a blocker!) to your use case.

Already done! ) I will look at the SQL REST API and perhaps we can use it. Otherwise we will fallback to postgress

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists status-triage_done Initial triage done, will be further handled by the driver team
Projects
None yet
Development

No branches or pull requests

2 participants