Skip to content

fix: import url from node to support domainToASCII#163

Open
SebouChu wants to merge 1 commit intofastify:mainfrom
SebouChu:fix-domain-to-ascii
Open

fix: import url from node to support domainToASCII#163
SebouChu wants to merge 1 commit intofastify:mainfrom
SebouChu:fix-domain-to-ascii

Conversation

@SebouChu
Copy link

No test was covering the usage of URL.domainToASCII which is failing in the current state.

The added test returns this without the fix:

not ok 59 host errors
  ---
    operator: equal
    expected: |-
      undefined
    actual: |-
      'Host\'s domain name can not be converted to ASCII: TypeError: URL.domainToASCII is not a function'
    at: Test.<anonymous> (/Users/sebastiengaya/_Dev/SebouChu/fast-uri/test/parse.test.js:47:5)
    stack: |-
      Error: host errors
          at Test.assert [as _assert] (/Users/sebastiengaya/_Dev/SebouChu/fast-uri/node_modules/tape/lib/test.js:492:48)
          at Test.strictEqual (/Users/sebastiengaya/_Dev/SebouChu/fast-uri/node_modules/tape/lib/test.js:670:7)
          at Test.<anonymous> (/Users/sebastiengaya/_Dev/SebouChu/fast-uri/test/parse.test.js:47:5)
          at Test.run (/Users/sebastiengaya/_Dev/SebouChu/fast-uri/node_modules/tape/lib/test.js:126:28)
          at Immediate.next [as _onImmediate] (/Users/sebastiengaya/_Dev/SebouChu/fast-uri/node_modules/tape/lib/results.js:158:7)
          at process.processImmediate (node:internal/timers:504:21)
  ...

So we import the url module from Node to support the function.

Open to any suggestions if needed, thanks!

Checklist

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina
Copy link
Member

CI is failing

@SebouChu
Copy link
Author

Oh, didn't think about the browser usage. Which let me thinking about why fast-uri uses a method that does not exist in browser environment?

@SebouChu
Copy link
Author

@mcollina What do you think about the idea of not converting the host to an ASCII representation, and keeps it in Unicode form?

The method currently does not work at all, and to stay in the "dependency-free" state, I think it's better to leave the host as it is.

@mcollina
Copy link
Member

Do you think you could provide a fallback for domainToASCII for browsers?

@SebouChu
Copy link
Author

In NodeJS, it's implemented in C code, so I don't really know how to implement natively here. If you're more familiar with it: https://github.com/nodejs/node/blob/main/src/node_url.cc#L184

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants