jsdom / tr46

An implementation of the Unicode UTS #46: Unicode IDNA Compatibility Processing.
MIT License
32 stars 14 forks source link

Update to spec version 10.0.0 + Revamp API #11

Closed TimothyGu closed 7 years ago

TimothyGu commented 7 years ago

Also update and make the entire test suite pass.

Sebmaster commented 7 years ago

In general: This is amazing stuff. Thank you so much for this!

TimothyGu commented 7 years ago

Converted the entire repo to Node.js v6+, also fixed a couple more test cases so that all of them pass (fixes #6). This should be ready to merge now.

TimothyGu commented 7 years ago

Just a note, maybe we should consider revising the API to be a bit more palatable, like (default value after =):

tr46.toASCII(domainName, {
  checkHyphens = false,
  checkBidi = false,
  checkJoiners = false,
  useSTD3ASCIIRules = false,
  // Perhaps also use a string or a symbol that's more self-documenting than a number
  processingOption = tr46.NONTRANSITIONAL,
  verifyDnsLength = false
});

rather than

tr46.toASCII(domainName, useSTD3, processingOption, verifyDnsLength, checkHyphens, checkBidi, checkJoiners);

Compatibility must be broken, of course, but since we are probably releasing a 1.0.0 after this PR anyway, it shouldn't be as big of a concern as it could be.

I'd be happy to implement this change.

Sebmaster commented 7 years ago

Compatibility must be broken, of course, but since we are probably releasing a 1.0.0 after this PR anyway, it shouldn't be as big of a concern as it could be.

Yeah, I think at this point the parameter list is getting a bit too long. Options argument would be very nice.

TimothyGu commented 7 years ago

@Sebmaster done.

domenic commented 7 years ago

@Sebmaster if we're fixing the public API anyway, WDYT of complying to the usual JS pattern for enums so you can do processingOption: "transitional" / processingOption: "nontransitional"?

Sebmaster commented 7 years ago

Since we're going that way already anyways. Might as well. Provide constants + make them simple strings which may be provided as well?

domenic commented 7 years ago

I've never been a fan of providing the constants myself but shrug.

domenic commented 7 years ago

As an aside, I wonder why they didn't make transitional processing another boolean flag...

Filed an issue with Unicode: https://github.com/whatwg/url/issues/53#issuecomment-309243069

Sebmaster commented 7 years ago

Also there are some unused vars now which we can drop?

TimothyGu commented 7 years ago

Constants removed.

Sebmaster commented 7 years ago

Seems like lint is still failing because of the unused vars.

TimothyGu commented 7 years ago

@Sebmaster Fixed, sorry.

TimothyGu commented 7 years ago

Tables updated to the released 10.0.0 standard.

TimothyGu commented 7 years ago

@Sebmaster ping