Would a PR to make azure stuff peerDependenciesMeta and maybe decrease some of the dependencies be acceptable?

[TheThing]

Hi there.

I was wondering if PR that would make some of the larger dependencies be inside peerDependenciesMeta instead of part of the dependency list would be accepted or rejected due to breaking changes?

Cause right now the dependency list for tedious is really really... big and most of it comes from integration with azure services:

 D:\NFP\temp> npm list
D:\NFP\temp
`-- tedious@11.0.9
  +-- @azure/identity@1.3.0
  | +-- @azure/core-http@1.2.5
  | | +-- @azure/abort-controller@1.0.4
  | | | `-- tslib@2.2.0 deduped
  | | +-- @azure/core-asynciterator-polyfill@1.0.0
  | | +-- @azure/core-auth@1.3.0
  | | | +-- @azure/abort-controller@1.0.4 deduped
  | | | `-- tslib@2.2.0 deduped
  | | +-- @azure/core-tracing@1.0.0-preview.11 deduped
  | | +-- @azure/logger@1.0.2 deduped
  | | +-- @types/node-fetch@2.5.10
  | | | +-- @types/node@15.12.0 deduped
  | | | `-- form-data@3.0.1 deduped
  | | +-- @types/tunnel@0.0.1
  | | | `-- @types/node@15.12.0 deduped
  | | +-- form-data@3.0.1
  | | | +-- asynckit@0.4.0
  | | | +-- combined-stream@1.0.8
  | | | | `-- delayed-stream@1.0.0
  | | | `-- mime-types@2.1.31
  | | |   `-- mime-db@1.48.0
  | | +-- node-fetch@2.6.1
  | | +-- process@0.11.10
  | | +-- tough-cookie@4.0.0
  | | | +-- psl@1.8.0
  | | | +-- punycode@2.1.1 deduped
  | | | `-- universalify@0.1.2
  | | +-- tslib@2.2.0 deduped
  | | +-- tunnel@0.0.6
  | | +-- uuid@8.3.2 deduped
  | | `-- xml2js@0.4.23
  | |   +-- sax@1.2.4
  | |   `-- xmlbuilder@11.0.1
  | +-- @azure/core-tracing@1.0.0-preview.11
  | | +-- @opencensus/web-types@0.0.7
  | | +-- @opentelemetry/api@1.0.0-rc.0
  | | `-- tslib@2.2.0 deduped
  | +-- @azure/logger@1.0.2
  | | `-- tslib@2.2.0 deduped
  | +-- @azure/msal-node@1.0.0-beta.6
  | | +-- @azure/msal-common@4.3.0
  | | | `-- debug@4.3.1
  | | |   `-- ms@2.1.2 deduped
  | | +-- axios@0.21.1 deduped
  | | +-- jsonwebtoken@8.5.1
  | | | +-- jws@3.2.2 extraneous
  | | | +-- lodash.includes@4.3.0
  | | | +-- lodash.isboolean@3.0.3
  | | | +-- lodash.isinteger@4.0.4
  | | | +-- lodash.isnumber@3.0.3
  | | | +-- lodash.isplainobject@4.0.6
  | | | +-- lodash.isstring@4.0.1
  | | | +-- lodash.once@4.1.1
  | | | +-- ms@2.1.2
  | | | `-- semver@5.7.1
  | | `-- uuid@8.3.2 deduped
  | +-- @types/stoppable@1.1.1
  | | `-- @types/node@15.12.0
  | +-- axios@0.21.1
  | | `-- follow-redirects@1.14.1
  | +-- events@3.3.0
  | +-- jws@4.0.0
  | | +-- jwa@2.0.0 extraneous
  | | `-- safe-buffer@5.2.1
  | +-- keytar@7.7.0
  | | +-- node-addon-api@3.2.1
  | | `-- prebuild-install@6.1.3
  | |   +-- detect-libc@1.0.3
  | |   +-- expand-template@2.0.3
  | |   +-- github-from-package@0.0.0
  | |   +-- minimist@1.2.5
  | |   +-- mkdirp-classic@0.5.3
  | |   +-- napi-build-utils@1.0.2
  | |   +-- node-abi@2.30.0
  | |   | `-- semver@5.7.1 deduped
  | |   +-- npmlog@4.1.2
  | |   | +-- are-we-there-yet@1.1.5
  | |   | | +-- delegates@1.0.0
  | |   | | `-- readable-stream@2.3.7 extraneous
  | |   | +-- console-control-strings@1.1.0
  | |   | +-- gauge@2.7.4
  | |   | | +-- aproba@1.2.0
  | |   | | +-- console-control-strings@1.1.0 deduped
  | |   | | +-- has-unicode@2.0.1
  | |   | | +-- object-assign@4.1.1
  | |   | | +-- signal-exit@3.0.3
  | |   | | +-- string-width@1.0.2
  | |   | | | +-- code-point-at@1.1.0
  | |   | | | +-- is-fullwidth-code-point@1.0.0
  | |   | | | | `-- number-is-nan@1.0.1
  | |   | | | `-- strip-ansi@3.0.1 deduped
  | |   | | +-- strip-ansi@3.0.1
  | |   | | | `-- ansi-regex@2.1.1
  | |   | | `-- wide-align@1.1.3
  | |   | |   `-- string-width@1.0.2 deduped
  | |   | `-- set-blocking@2.0.0
  | |   +-- pump@3.0.0
  | |   | +-- end-of-stream@1.4.4
  | |   | | `-- once@1.4.0 deduped
  | |   | `-- once@1.4.0
  | |   |   `-- wrappy@1.0.2
  | |   +-- rc@1.2.8
  | |   | +-- deep-extend@0.6.0
  | |   | +-- ini@1.3.8
  | |   | +-- minimist@1.2.5 deduped
  | |   | `-- strip-json-comments@2.0.1
  | |   +-- simple-get@3.1.0
  | |   | +-- decompress-response@4.2.1
  | |   | | `-- mimic-response@2.1.0
  | |   | +-- once@1.4.0 deduped
  | |   | `-- simple-concat@1.0.1
  | |   +-- tar-fs@2.1.1
  | |   | +-- chownr@1.1.4
  | |   | +-- mkdirp-classic@0.5.3 deduped
  | |   | +-- pump@3.0.0 deduped
  | |   | `-- tar-stream@2.2.0
  | |   |   +-- bl@4.1.0 deduped
  | |   |   +-- end-of-stream@1.4.4 deduped
  | |   |   +-- fs-constants@1.0.0
  | |   |   +-- inherits@2.0.4 deduped
  | |   |   `-- readable-stream@3.6.0 deduped
  | |   `-- tunnel-agent@0.6.0
  | |     `-- safe-buffer@5.2.1 deduped
  | +-- msal@1.4.11
  | | `-- tslib@1.14.1 extraneous
  | +-- open@7.4.2
  | | +-- is-docker@2.2.1
  | | `-- is-wsl@2.2.0
  | |   `-- is-docker@2.2.1 deduped
  | +-- qs@6.10.1
  | | `-- side-channel@1.0.4
  | |   +-- call-bind@1.0.2
  | |   | +-- function-bind@1.1.1
  | |   | `-- get-intrinsic@1.1.1 deduped
  | |   +-- get-intrinsic@1.1.1
  | |   | +-- function-bind@1.1.1 deduped
  | |   | +-- has@1.0.3
  | |   | | `-- function-bind@1.1.1 deduped
  | |   | `-- has-symbols@1.0.2
  | |   `-- object-inspect@1.10.3
  | +-- stoppable@1.1.0
  | +-- tslib@2.2.0
  | `-- uuid@8.3.2
  +-- @azure/keyvault-keys@4.1.0
  | +-- @azure/core-http@1.2.5 deduped
  | +-- @azure/core-lro@1.0.5
  | | +-- @azure/abort-controller@1.0.4 deduped
  | | +-- @azure/core-http@1.2.5 deduped
  | | +-- @azure/core-tracing@1.0.0-preview.11 deduped
  | | +-- events@3.3.0 deduped
  | | `-- tslib@2.2.0 deduped
  | +-- @azure/core-paging@1.1.3
  | | `-- @azure/core-asynciterator-polyfill@1.0.0 deduped
  | +-- @azure/core-tracing@1.0.0-preview.9 extraneous
  | +-- @azure/logger@1.0.2 deduped
  | +-- @opentelemetry/api@0.10.2 extraneous
  | `-- tslib@2.2.0 deduped
  +-- @azure/ms-rest-nodeauth@3.0.10
  | +-- @azure/ms-rest-azure-env@2.0.0
  | +-- @azure/ms-rest-js@2.5.0
  | | +-- @azure/core-auth@1.3.0 deduped
  | | +-- abort-controller@3.0.0
  | | | `-- event-target-shim@5.0.1
  | | +-- form-data@2.5.1 extraneous
  | | +-- node-fetch@2.6.1 deduped
  | | +-- tough-cookie@3.0.1 extraneous
  | | +-- tslib@1.14.1 extraneous
  | | +-- tunnel@0.0.6 deduped
  | | +-- uuid@3.4.0 extraneous
  | | `-- xml2js@0.4.23 deduped
  | `-- adal-node@0.2.2 deduped
  +-- adal-node@0.2.2
  | +-- @types/node@8.10.66 extraneous
  | +-- async@2.6.3
  | | `-- lodash@4.17.21
  | +-- axios@0.21.1 deduped
  | +-- date-utils@1.2.21
  | +-- jws@3.2.2 extraneous
  | +-- underscore@1.13.1
  | +-- uuid@3.4.0 extraneous
  | +-- xmldom@0.6.0
  | `-- xpath.js@1.1.0

I was wondering if a PR that refactored some of the internal mechanics made those peerDependenciesMeta optional like knex does with its drivers would be acceptable to this community? Cause not everyone is gonna be using azure and right now:

That's over five thousand files of code installed and 32MB just to talk with a database. And personally I find that a tad bit too much for a single dependency in production environment.

If such a thing is acceptable, I would get started on making that PR to... reduce the size and increase the nimbleness of tedious if that's okay. If not then feel free to close this issue :)

[kurtinatlanta]

Worse - it appears that this patch release adds a dependency on @azure/identity 1.3.0, which has a transitive dependency on something called 'keytar', that requires a download and compile of some C code that wasn't there earlier. Our build systems cannot download files from AWS, so this part of the install fails our builds.

Is there a need to download this C code?

[TheThing]

could we have an input on this? I would like to know if I should try and maintain compatibility with current API or if I should just go at it with a sledgehammer, delete a bunch of code and then just publish a -lite after the name like I've done million times before.

[IanChokS]

@arthurschreiber I think this would be more of your call. Any thoughts?

[gpetrov]

Please add Azure and specially @azure/identity as peer dependency! Otherwise we get indeed tons of modules and a native module that can't be compiled always. So Tedious is no longer light js only connector to MS SQL!

[TheThing]

@gpetrov I've actually cancelled working on a PR for this project that has over five thousand files and instead just oped to use micrsoft's own driver: node-sqlserver It's a single dependancy and looks far better on paper. (if you remove that silly binary downloader and have the binary already in the folder, it doesn't depend on anything outside to run).

So I'm closing this issue personally as there was no input from arthur and no updates.

[arthurschreiber]

Hey @TheThing @gpetrov @kurtinatlanta,

I'm sorry I lost track of this, and I'm sorry that I made you feel ignored. I understand the dependency situation is not great and there's been a few people who've raised concerns about this. Plus, having more direct dependencies means also that there's more code that potentially can contain security issues (see the recent security problem in node-adal coming from xmldom), so I'd be absolutely on board with improving this situation.

I tried defining an authentication plugin API over at https://github.com/tediousjs/tedious/pull/624 years ago, but that just never turned out to be a good solution. I think that mostly was caused by that PR trying to abstract too much.

There's essentially 3 authentication methods in the TDS protocol:

• SQL Server authentication (username / password)
• SSPI authentication (NTLM / Kerberos / Negotiate / Integrated Auth)
• Federated Authentication (Azure Active Directory / Token Auth)

In theory, you can connect to a server and provide all 3 authentication options at the same time, and the server will pick out whatever it wants to use.

Maybe it makes sense to have 2 plugin interfaces? One for SSPI and the other for Federated Authentication? https://github.com/tediousjs/tedious/pull/624 was a failure because it only abstracted one (SSPI), and when we added Federated Authentication for Azure support, I couldn't come up with a good way of having it cover both authentication systems and just gave up. Now I think it just does not make sense to have one system cover both, and having two might be the better approach?

I don't currently have a good suggestion how this could look implementation wise, but if you have an idea, I'm all ears! What do you think?

[TheThing]

I didn't feel ignored. In fact I'm quite happy I don't have to do this since that would be a ton of work and node-sqlserver is already single-dependency with very few tens of files totals so it was perfect for my need.

It was more of a relief that this dragged on as it gave me time to google and find that package <3

[TheThing]

that being said, plugin-based architecture for authentication is what makes most sense to me. Not everoyne is gonna be using SSPI and those using that might not use Azure and etc. This would also simply tedious to be basically focus on supporting the features and the authentication can be elsewhere. But then again, I personally loathe plugin-based npm as they always end up requiring me to install soo many modules just to get a working prototype but that's a very personal opinion of mine which nobody agrees with me on so I digress :b

[gpetrov]

@arthurschreiber the solution is actually very simple. Just put @azure/identity as optional peer dependency. People that need it - just have to install it manually.

So in package.json remove it from the regular dependencies and just add it as:

"peerDependenciesMeta": {
    "@azure/identity": {
      "optional": true
    }

Maybe more packages can be added in such way as optional.

[arthurschreiber]

@arthurschreiber the solution is actually very simple. Just put @azure/identity as optional peer dependency. People that need it - just have to install it manually.

But then we also need to update the code to handle this being an optional dependency correctly. And I recently learned that there's the people that use bundlers where dynamic imports are not really supported well. 🤷

[gpetrov]

@arthurschreiber optional dependencies are very common and used in many projects. There is absolutely no problem with them and using them is exactly the reason they exist! To cut down the optional dependencies.

A good example of optional dependencies is https://github.com/knex/knex where they use optional dependencies for each database driver, as you usually need just one driver and not all supported.

Same should be with Tedious - just add the authentication methods you want - and not all possible.

Knex actually use Tedious as well because of its early light dependencies, but now with all those tons of unnecessary dependencies it is getting harder to justify. Specially on production environments.

[laurisvan]

We have also recently faced this issue when using tedious via Lambda functions. In such cases the number of dependencies matter - both for cold start of the Lambdas (bigger packages take more time to load) and stability of the infrastructure (uploading large Lambda functions fail).

If the azure etc. dependencies are not absolute must haves, it would be great to have them as optional deps, instead.

[TheThing]

@laurisvan you might wanna check out node-sqlserver-v8: https://github.com/TimelordUK/node-sqlserver-v8 It does require ODBC drivers to be installed but if you can get that, then it's a much smaller package. And if you fiddle with it a little bit you can get it reduced to literal zero dependency (which is something I'm working on).

[TheThing]

Maybe removing this from the README is a good idea? ;)

It is intended to be a fairly slim implementation of the protocol, with not too much additional functionality.

[laurisvan]

@TheThing

@laurisvan you might wanna check out node-sqlserver-v8: https://github.com/TimelordUK/node-sqlserver-v8 It does require ODBC drivers to be installed but if you can get that, then it's a much smaller package. And if you fiddle with it a little bit you can get it reduced to literal zero dependency (which is something I'm working on).

Unfortunately I don't have that option. I am using tedious via knex package and writing a new knex driver adapter would be a bit overwhelming right now. We changed our infra code to cope with bigger Lambdas, so at least we will now get the big packages through the build pipeline.

[mastodon0]

It's hacky but good enough if you have no other options. You can replace the Azure dependencies via npm-overrides in the package.json with something like this

"overrides": {
    "tedious": {
        "@azure/identity": "npm:useless-module@1.0.3",
        "@azure/keyvault-keys": "npm:useless-module@1.0.3"
    }
}

It shrinks the node_modules by ~20mb and works as long as you don't need any Azure features.

[TheThing]

This is the single most greatest comment I've ever seen. Unfortunately I've moved over to a different package that's much much smaller. Thanks anyways :D