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.

Sign up for GitHub

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

Jump to bottom

fix: use userland module for punycode #1596

Merged
merged 2 commits into from
Jan 31, 2024
Merged

fix: use userland module for punycode #1596

merged 2 commits into from
Jan 31, 2024

Conversation

EliTheCoder
Copy link
Contributor

@EliTheCoder EliTheCoder commented Jan 17, 2024 •
edited
Loading

The slash at the end is necessary to use the userland module. The module built-in to node is deprecated.

Use userland module for punycode
2f63fa1 
The slash at the end is necessary to use the userland module
@EliTheCoder EliTheCoder changed the title Use userland module for punycode fix: Use userland module for punycode Jan 17, 2024
@codecov Codecov
Copy link

codecov bot commented Jan 23, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (57998f2) 79.00% compared to head (d6fb8c9) 78.07%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1596      +/-   ##
==========================================
- Coverage   79.00%   78.07%   -0.94%     
==========================================
  Files          93       93              
  Lines        4821     4821              
  Branches      921      921              
==========================================
- Hits         3809     3764      -45     
- Misses        709      756      +47     
+ Partials      303      301       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MichaelSun90
Copy link
Contributor

Hi @EliTheCoder, Thanks for raising this PR. I also tried to use the url.domainToASCII function to replace the one that used to be provided by punnyCode. Seems the related tests still behave the same way. Not sure whether we want still keep the punnyCode or just use url instead.
Hi @arthurschreiber, seems url.domainToASCII can handle what punycode.toASCII used to do. Do we want still keep punnyCode or just go with the url lib?

@MichaelSun90
Copy link
Contributor

Just double checked that Node 14 seems do not support node:url but we should be safe since the minimum node version maintained on tedious side is node 16 🤔 .

@arthurschreiber arthurschreiber changed the title fix: Use userland module for punycode fix: use userland module for punycode Jan 31, 2024
@arthurschreiber arthurschreiber changed the title fix: use userland module for punycode fix: use userland module for punycode Jan 31, 2024
@arthurschreiber arthurschreiber merged commit 6a961b5 into tediousjs:master Jan 31, 2024
31 of 32 checks passed
@arthurschreiber
Copy link
Collaborator

Thanks for your contribution! ❤️

@github-actions GitHub Actions
Copy link

🎉 This PR is included in version 16.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

KAMAELUA pushed a commit to KAMAELUA/tedious-decimal that referenced this pull request Apr 29, 2024
@EliTheCoder @KAMAELUA
fix: use userland module for punycode (tediousjs#1596)
b960684 
* Use userland module for punycode

The slash at the end is necessary to use the userland module

* fix: Add @types/punycode as dev dependency

---------

Co-authored-by: Eli Frigo <[email protected]>
(cherry picked from commit 6a961b5)
@titobundy titobundy mentioned this pull request May 26, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@EliTheCoder @MichaelSun90 @arthurschreiber