search

tediousjs / tedious

Node TDS module for connecting to SQL Server databases.
http://tediousjs.github.io/tedious/
MIT License
1.56k stars 443 forks source link

refactor: replace `punycode` with `node:url` #1597

Closed MichaelSun90 closed 4 months ago

MichaelSun90 commented 4 months ago

Seems that native node:url supports the same functionality that tedious required. Replace punnyCode side function with url.domainToASCII.

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (19cb073) 66.05% compared to head (4ca3043) 65.99%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1597 +/- ## ========================================== - Coverage 66.05% 65.99% -0.07% ========================================== Files 93 93 Lines 4858 4858 Branches 932 932 ========================================== - Hits 3209 3206 -3 Misses 1245 1245 - Partials 404 407 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

arthurschreiber commented 4 months ago

Can you update the PR to also drop the dependency on the punycode module? 🙇‍♂️

github-actions[bot] commented 4 months ago

:tada: This PR is included in version 16.7.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

WikiRik commented 4 months ago

Hi there! Coming over here from sequelize, a database ORM which is using this package as the adapter for mssql. This PR broke one of our tests. We were using 'http://wowow.example' as the host, which punycode kept intact (since there all characters were already ASCII). Node however returned an empty string since it is not a valid domain (due to the usage of 'http://'). Luckily that's an easy fix for us, but others might stumble over that.

We did also get this DeprecationWarning from Node with that, because the hostname that it tried to lookup was empty. It might be nice to catch that in advance and throw an error that the host given is not a valid domain. https://github.com/tediousjs/tedious/blob/ddfa397bf8ed4a4d2ded46d98c4a7bed2e37dd72/src/connector.ts#L170

MichaelSun90 commented 4 months ago

Hi @WikiRik , Thanks for sharing your feedback. For the DeprecationWarning, I did a bit investigation. Seems this is returned by the lookup function relate to this: DEP0118: dns.lookup() support for a falsy host name. The description mentions this will become an error in the future. We will discuses this, make sure proper error is thrown and see if we need to do any additional handling on this.