tediousjs / node-mssql

Microsoft SQL Server client for Node.js
https://tediousjs.github.io/node-mssql
MIT License
2.23k stars 466 forks source link

NodeJs 20 support #1540

Closed ahkelly closed 1 year ago

ahkelly commented 1 year ago

Any guidance on when NodeJs 20 support will be added?

Actual behaviour:

npm install mssql

generates the following

npm ERR! code EBADENGINE npm ERR! engine Unsupported engine npm ERR! engine Not compatible with your version of node/npm: @azure/msal-node@1.18.2 npm ERR! notsup Not compatible with your version of node/npm: @azure/msal-node@1.18.2 npm ERR! notsup Required: {"node":"10 || 12 || 14 || 16 || 18"} npm ERR! notsup Actual: {"npm":"9.8.0","node":"v20.5.0"}

Software versions

dhensby commented 1 year ago

Those dependencies aren't direct dependencies of this repo.

That error also should not actually prevent the install of the dependencies.

LucasMonteiro1 commented 1 year ago

Package @azure/msal-node has published new version with support for node 20

https://www.npmjs.com/package/@azure/msal-node#node-version-support

ahkelly commented 1 year ago

What the easiest way to npm install and ignore this msal-node nightmare that keeps throwing this error whenever I try and install anything on this project. I am not even using azure auth. The latest node msal package is 1.18.3 - it is version 2.* that has node 20 support.

dhensby commented 1 year ago

I'm not sure what the problem is. When I install locally I get warnings, but not errors about the bad engine, and so the operation continues without issue.

Is this problem actually stopping you from installing or is it just that you don't like the errors being output?

If you don't like the errors, you've got a couple of options:

  1. Open a PR against tedious to update the msal-node lib to 2.x and wait for that to be merged/released
  2. Use node 18
ahkelly commented 1 year ago

Yeah so you must be using Node 18. I am on Node 20 as it will be LTS any day now and everything I use (svelte, prisma) has no issue with 20.

node -v
#V.20.5.0
npm install mssql

I get the output

npm ERR! code EBADENGINE
npm ERR! engine Unsupported engine
npm ERR! engine Not compatible with your version of node/npm: @azure/msal-node@1.18.3
npm ERR! notsup Not compatible with your version of node/npm: @azure/msal-node@1.18.3
npm ERR! notsup Required: {"node":"10 || 12 || 14 || 16 || 18"}
npm ERR! notsup Actual:   {"npm":"9.8.0","node":"v20.5.0"}

Then whenever you install / update a new package using npm install it checks the package.json and throws this error again and randomly prevents other packages installing if they come after mssql. So basically I have to uninstall mssql, install the new package, re-install mssql. To say that is a pain in the a$$ is an understatement. I realize this is a Microsoft issue so it would be nice to be to install without the msal-node dependency as we don't need it with a private SQL Server instance.

I am just reverting to using Prisma as it weirdly seems faster than then tedious on most requests ?? and my production server -> SQL Server I get connection issues with node-mssql but not Prisma so I fall back to using Prisma and keep checking back here for updates and try , try again :)

dhensby commented 1 year ago

Yeah so you must be using Node 18.

Why would I be getting bad engine warnings on Node 18 if it is one of the explicitly supported versions?

$ node --version
v20.6.1
$ test npm --version 
9.8.1
$ npm i mssql@9
npm i mssql@9
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '@azure/msal-node@1.18.3',
npm WARN EBADENGINE   required: { node: '10 || 12 || 14 || 16 || 18' },
npm WARN EBADENGINE   current: { node: 'v20.6.1', npm: '9.8.1' }
npm WARN EBADENGINE }

After searching, it looks like you may have engine-strict=true enabled in your npm config. You can check using npm config get engine-strict - my environment returns false.

ahkelly commented 1 year ago

OK, thank you for that. I did find my environment has this set to false. I am leaning on possibly downgrading back to node 18 and give this another shot to see if everything else in my build process is good with 18 and stop fighting with it. Thanks again for your guidance.

ahkelly commented 1 year ago

Works like a champ in Node 18!

ahkelly commented 11 months ago

Sorry to bug you again about this but NodeJs 20 is now lts and this issue still exists. This package is attempting to install an old version of @azure/msal-node@1.18.3. Version 2 is available and fully supports Nodejs 20 and can be installed separately without issue. Please update the dependencies. Thank you kindly for your efforts.

dhensby commented 11 months ago

As mentioned, this is not a direct dependency of this repo. I cannot do anything to update it.

You can fix NPM to allow these "unsupported" engines.

There is no action I can take, you'll have to raise it with the upstream library.


Edit: And to clarify, the test suite runs (and passes) on node 20, so there is no problem with running on Node 20

ahkelly commented 11 months ago

Understood, I figured as much but am unable to decipher "who" the upstream culprit is that is trying to install the outdated version of msal - I will raise with them if you would direct me :) The fact that the exact error that I noted in the original request still exists with this package as a result I would think would be a concern for you. There may be no problem running on node 20 but you "cannot" even install it without using the "hacks" you note.

Thank you for your patience as I know anyone not using Node 18 will be dealing with this.

dhensby commented 11 months ago

npm ls @azure/msal-node will point you in the right direction.

I wouldn't say the solution is a "hack" as it is the default behaviour. It erroring is not default. you can see in our CI there are no tricks to get the dependencies installing.

I can't concern myself with all the edge-cases of transitive dependencies. I can only concern myself with the library I maintain and possibly its nearest neighbours, but certainly not dependencies many level deeps. If I concerned myself with all the dependencies, I'd be maintaining hundreds of packages.

ahkelly commented 11 months ago

I 100% understand and agree with you. I have brought it up with Tedious, who I am sure will say its Azure (which it is). My point, which I understand and respect that you disagree with, has always been as of today using a default installation of NodeJs 20 (on linux) your excellent package cannot be installed (and does indeed error) and that this disabling feature of npm is not documented anywhere on this site. I started developing this new application using Prisma as a result of this and who knows how many others not knowing how much better the experience is using this package natively.

FYI - The azure team are working on a release imminently to make this problem hopefully go away.

https://github.com/Azure/azure-sdk-for-js/issues/26886

dhensby commented 11 months ago

It's not whether I agree or disagree about whether there's an issue installing this library on node 20, it's that this is an issue out of my hands.

I'm trying to help with workarounds, and also pointing out this isn't a problem in our CI, but beyond that I can't control the limitations of deeply nested dependencies, especially ones of tedious which is a fundamental requirement of this library to work. We can only wait for updates to those libraries that resolve the problem.

vaunus commented 10 months ago

Tedious has added Node 20 support now so I guess bumping the dependency would fix this?

https://github.com/tediousjs/tedious/releases/tag/v16.6.1

reubenrubiogti commented 10 months ago

@dhensby Just making sure you saw the above comment! Node 20 support would be fantastic.

dhensby commented 10 months ago

This project requires tedious@^16, 16.6.1 is within that range so it'll work as is 💪 - just update your nested dependencies (npm update should do it)

dhensby commented 10 months ago

Just to clarify of anyone coming across this issue (and before yet another duplicate issue is opened).

Dependency resolution does not require that a new version of this library is released.

The current dependency on tedious is ^16.4.0, which means tedious@16.6.1 is perfectly compatible.

$ mkdir ~/test
$ cd ~/test
$ npm i mssql
$ npm ls tedious
test@ ~/test
└─┬ mssql@10.0.1
  └── tedious@16.6.1

If your local version is still not installing the latest version, you will either need to use npm update, or try to install without any node_modules dir (npm can be lazy and if a version is already present, it won't update it if it remains compatible).

rujorgensen commented 7 months ago

Can someone please ELI5, why one guy after another in here keeps saying everything is alright, yet when I run pnpm i, I keep getting this 🙂. Sorry, I (and seemingly a number of people) simply don't get it 😊.

image

I have engines.node: "20.11.1" in my package.json, and .npmrc like below, because as good citizens we want to use the latest LTS of NodeJS:

engine-strict=true
use-node-version=20.11.1

What do I have to change to make this work? (downgrading to a lower LTS is not an option.)

Thanks!

dhensby commented 7 months ago

You have engine-strict=true so the behaviour you're seeing is expected/intended and working as your configuration is asking it to. You're making it so you can't install the software and then asking why you can't install the software.

You package manager is also trying to install v16.4.1 of tedious instead of the latest v16 (which is now v16.7.1) - I don't know why you're package manager is acting that way, ask the pnpm maintainers.

Hopefully people understand it's not reasonable for packages that have dependencies to author releases every single time a dependency has a new version, that's the entire reason for the semver operators (~ and ^) which allow packages to define broad versions of dependencies.

I have also provided some potential steps to resolve the issue:

If your local version is still not installing the latest version, you will either need to use npm update, or try to install without any node_modules dir (npm can be lazy and if a version is already present, it won't update it if it remains compatible).

Did you try that?

rujorgensen commented 7 months ago

@dhensby Thanks for your detailed answer 🙂.

You have engine-strict=true so the behaviour you're seeing is expected/intended and working as your configuration is asking it to.

Hmm, maybe I'm not understanding how engine-strict=true is supposed to work 🤔. We added it because we want to enforce compliance with the NodeJS version that will be available at runtime. It seems sane to me to refuse dependency on an older version. For msal-node to have 10 || 12 || 14 || 16 ||18 (not including 20) seems to be the issue in my eyes (I know that's not a library you're controlling though).

You package manager is also trying to install v16.4.1 of tedious instead of the latest v16 (which is now v16.7.1)

I'll look into that, thanks!

Hopefully people understand it's not reasonable for packages that have dependencies to author releases every single time a dependency has a new version

Absolutely, good point 👍.

The problem seems to be gone after running npm update, even if I revert back to the original pnpm-lock.yaml, and try to reinstall, I don't see the error again which is surprising because then it seems to be an issue with pnpm, and which packages it has available globally 🫤.

By running npm update I'm just worried that we end up in a situation where everyone in my team has to run this, or forcefully remove node_modules every time we update dependencies. I'd like it to just work as it always have: update deps., run pnpm i, test, commit, done.

dhensby commented 7 months ago

I think the root issue is that the package managers (seemingly both npm and pnpm) are not always trying to install the latest eligible package for (nested) dependencies, and sometimes will just install the minimum compatible version.

I don't know the exact reason behind this, but given an npm update or removing the node_modules folder seems to "fix" the issue (ie: it does then install the latest compatible version), I'd speculate that the package managers are trying to be "efficient" by essentially seeing that a compatible version is already installed and so doesn't bother to evaluate for a new version. Basically npm install means "install packages that are compatible with my constraints", and npm update means "install the latest packages that are compatible with my constraints".

By running npm update I'm just worried that we end up in a situation where everyone in my team has to run this, or forcefully remove node_modules every time we update dependencies. I'd like it to just work as it always have: update deps., run pnpm i, test, commit, done.

This is what the package-lock.json file and npm ci are for; that will guarantee that everyone has the exact same dependencies installed.

The problem seems to be gone after running npm update, even if I revert back to the original pnpm-lock.yaml, and try to reinstall, I don't see the error again which is surprising because then it seems to be an issue with pnpm, and which packages it has available globally 🫤.

Well, that is odd - I can't speak to the pnp-lock.yaml and how it works with nested dependencies (I'm not experienced with pnpm) - are you using the equivalent of npm ci when reverting (which is saying to install from the lock file and not to perform any dependency resolution)?

Finally, to touch on engine-strict, I think you're understanding of it is spot on, unfortunately we don't live in a world with perfect package maintenance and you'll find yourself in exactly the position you have been, where a dependency hasn't got accurate or up-to-date engine declaration despite being perfectly compatible with the runtime (or, perhaps it's not compatible, but you don't use the dependency so it's of no consequence). The engine checks are a blunt tool, and you are experiencing that.