Update to spec version 10.0.0 + Revamp API by TimothyGu · Pull Request #11 · jsdom/tr46

TimothyGu · 2017-06-01T11:57:51Z

Also update and make the entire test suite pass.

Sebmaster · 2017-06-07T22:48:10Z

+  // A.1. ZWNJ, Rule 3
+  validZWNJ: `(?:${regenerate().add(cp.JT.L).add(cp.JT.D)})(?:${regenerate(cp.JT.T)})*` +
+             "\\u200C" +
+             `(?:${regenerate(cp.JT.T)})*(?:${regenerate([...cp.JT.R, ...cp.JT.D])})`,


Node v4 doesn't support expansion so this doesn't work. .concat?

I think I spotted some .includes() usage elsewhere. Maybe best to just require Node v6. That could also allow using default params.

Yeah, let's do that. Also allows us to use Array.from for symbol counting.

Sebmaster · 2017-06-07T22:51:40Z


  if (verifyDnsLength) {
-    const total = labels.slice(0, labels.length - 1).join(".").length;
-    if (total.length > 253 || total.length === 0) {


The spec says

The length of the domain name, excluding the root label and its dot, is from 1 to 253.

Am I misunderstanding the spec here?

total is already a number -- the length of the domain name excluding the root label and its dot.

Oh, I misunderstood your question. This one had me confused as well. The "root label" the spec is referring to is the implied empty label that all domain names have, but is generally omitted with its dot. However it is present in output from tools like dig (note the trailing dot after github.com):

;; QUESTION SECTION: ;github.com. IN A ;; ANSWER SECTION: github.com. 30 IN A 192.30.255.113 github.com. 30 IN A 192.30.255.112

Since it is usually omitted, we don't have to do anything extra to account for that.

What'd the root label be in this case? Would it be a TLD, or rather a empty label as in the DNS root zone?

Ah, I see. 👍

Sebmaster · 2017-06-07T22:52:38Z

+        error: true
+      };
+    }
+    if (cp > 0xFFFF) {


What's this one for?

We are looking for code points, but the .codePointAt() call against a trailing surrogate returns that surrogate itself. Therefore we have to manually skip that.

We should only iterate full codepoints here, so I don't think we should ever get to a trailing surrogate?

Let's take the string "a\uD800\uDC00b". When i = 1, codePointAt returns the code point that's > 0xFFFF. However, without the manual increment, when i = 2 codePointAt would return 0xDC00. That's just how codePointAt works.

We could, however, use for-of loop against the string which would handle this nuance for us.

Oh. Ew, I was not aware of that. I was also not aware for...of for astral strings has been supported for so long in node. Let's use that instead.

Sebmaster · 2017-06-07T22:53:18Z

+  }
+
+  if (checkHyphens) {
+    if (label[2] === "-" && label[3] === "-" ||


Parens here for more clarity

Sebmaster · 2017-06-07T22:54:52Z

  return null;
 }

-const regexAstralSymbols = /[\uD800-\uDBFF][\uDC00-\uDFFF]/g;


This breaks countSymbols?

Oops yeah it seems like it.

Sebmaster · 2017-06-07T22:59:51Z

+  bidiS1RTL: regenerate().add(cp.R).add(cp.AL),
+
+  // Step 2
+  bidiS2: `^(?:${regenerate()


Do we need the group in this one at all?

Yep. The output from regenerate() has the format [...]|\u....[...]|\u....[...]. Without the grouping the meaning would change when we append the * operator.

Sebmaster · 2017-06-07T23:01:09Z

+      // 2-4
+      if (!regexes.bidiS2.test(label) ||
+          !regexes.bidiS3.test(label) ||
+          regexes.bidiS4EN.test(label) && regexes.bidiS4AN.test(label)) {


Parens to make this clearer.

Sebmaster · 2017-06-07T23:01:32Z

+  bidiS4AN: regenerate(cp.AN),
+
+  // Step 5
+  bidiS5: `^(?:${regenerate()


Group necessary?

Sebmaster · 2017-06-07T23:03:09Z

+      };
+    }
+
+    if (rtl) {


Can we switch these around and handle ltr (the assumedly more common case) first? Maybe also rename the variable for it?

I followed this order since it's also the order used by the spec, but I'm okay with that.

Ah true. Let's leave it as-is then.

Sebmaster · 2017-06-07T23:05:09Z

+    }
+  }
+
+  if (checkJoiners) {


Add the spec link to this one: https://tools.ietf.org/html/rfc5892#appendix-A.

Is this only a partial implementation in general? What's with all the other characters (non-ZWNJ, non-ZWJ)?

The other characters in that appendix don't apply to CONTEXTJ. See https://tools.ietf.org/html/rfc5892#appendix-B.1.

What's with the chars in Appendix A.3 - A.9?

They are for other contexts. See the chart I linked. For example A.3 (U+00B7) is for CONTEXTO.

Sebmaster · 2017-06-07T23:09:41Z

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

TimothyGu · 2017-06-08T09:18:13Z

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 · 2017-06-09T07:15:19Z

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 · 2017-06-15T22:13:14Z

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.

Sebmaster

Reviewed the code: This is some pretty amazing work.

If you wanna do the options object too, I'll leave it to you, but either way this is good to merge.

TimothyGu · 2017-06-16T02:18:00Z

@Sebmaster done.

Sebmaster · 2017-06-17T21:24:56Z

-function processing(domainName, useSTD3, checkHyphens, checkBidi, checkJoiners, processingOption) {
+function processing(domainName, options) {
+  const { checkHyphens, checkBidi, checkJoiners, processingOption } = options;
+  if (processingOption !== TRANSITIONAL && processingOption !== NONTRANSITIONAL) {


Should do this in the public API, only toASCII requires it I think?

domenic · 2017-06-17T21:48:26Z

@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 · 2017-06-17T21:49:40Z

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

domenic · 2017-06-17T21:51:31Z

+  checkJoiners = false,
+  useSTD3ASCIIRules = false,
+  processingOption = NONTRANSITIONAL,
+  verifyDnsLength = false


Per https://w3ctag.github.io/design-principles/#casing-rules should be verifyDNSLength

The spec says VerifyDnsLength, but I'm happy to change this.

domenic · 2017-06-17T21:51:57Z

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

domenic · 2017-06-17T21:53:18Z

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

Filed an issue with Unicode: whatwg/url#53 (comment)

Sebmaster · 2017-06-18T02:26:19Z

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

TimothyGu · 2017-06-18T09:11:03Z

Constants removed.

Sebmaster · 2017-06-18T20:44:14Z

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

TimothyGu · 2017-06-20T02:27:30Z

@Sebmaster Fixed, sorry.

Also: - Make all tests pass. - Modernize code base to Node.js v6+. Fixes: #6

TimothyGu · 2017-06-21T10:17:06Z

Tables updated to the released 10.0.0 standard.

TimothyGu · 2017-07-24T03:26:50Z

@Sebmaster ping

Sebmaster requested changes Jun 7, 2017

View reviewed changes

domenic mentioned this pull request Jun 8, 2017

Feature request: exposing more ICU functionality or data? nodejs/node#13550

Closed

Sebmaster approved these changes Jun 15, 2017

View reviewed changes

Sebmaster requested changes Jun 17, 2017

View reviewed changes

domenic reviewed Jun 17, 2017

View reviewed changes

TimothyGu added 2 commits June 21, 2017 03:14

Implement version 10.0.0 / rev. 19

39bb1cf

Also: - Make all tests pass. - Modernize code base to Node.js v6+. Fixes: #6

Revamp API

a659528

TimothyGu changed the title ~~Update to latest spec draft~~ Update to spec version 10.0.0 + Revamp API Jun 21, 2017

Sebmaster merged commit a19e9c5 into jsdom:master Jul 27, 2017

TimothyGu deleted the feature/rev-18 branch July 28, 2017 00:54

Uh oh!

Uh oh!

Conversation

TimothyGu commented Jun 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sebmaster commented Jun 7, 2017

Uh oh!

TimothyGu commented Jun 8, 2017

Uh oh!

TimothyGu commented Jun 9, 2017

Uh oh!

Sebmaster commented Jun 15, 2017

Uh oh!

Sebmaster left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Jun 16, 2017

Uh oh!

domenic commented Jun 17, 2017 •

edited

Loading