Skip to content

Conversation

@patricksevat
Copy link
Collaborator

The current implementation has several imports which are not browser friendly:

  1. require('url') in encoder/index
  2. require('querystring') in index
  3. require('url') in legacy

This PR fixes those by providing a browser entrypoint which is mostly based on the browser's URL api: https://developer.mozilla.org/docs/Web/API/URL while trying to keep the api and results the same.

That said, there are some breaking changes and behavioral differences:

BREAKING

Behavorial changes, only applicable to the BROWSER ENTRYPOINT

  • .encodeQueryString with multi values behaves differently:
encodeQueryString({
            q1: ['𝌆й', '你ス'],
            q2: ''
        })
  • NodeJS: encode to the equivalent of 'q1=𝌆й&q1=你ス&q2=' (note the double q1 param)
  • Browser: encode to the equivalent of 'q1=𝌆й,你ス&q2=' (single, comma-separated q1 param)
  • invalid URLs are dealt with differently: toNodeUrl('xn--iñvalid.com')
    • NodeJS returns host: 'xn--iñvalid.com',
    • Browser returns: host: '-xn--xn--ivalid-x9a.com'

(typeof window === 'undefined' ? it : it.skip)('should return input value on invalid domain', function () {
expect(encoder.encodeHost('xn:')).to.equal('xn:');
expect(encoder.encodeHost('example#com')).to.equal('example#com');
// expect(encoder.encodeHost('example#com')).to.equal('example#com');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In newer Node versions (I used Node 22), this breaks

'HTTP://example.com',
'http://xn--48jwgn17gdel797d.com',
'http://xn--iñvalid.com',
// 'http://xn--iñvalid.com',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In newer Node versions, this breaks

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