util: expose the WHATWG Encoding API globally #20662

MarkTiedemann · 2018-05-10T17:26:08Z

This PR makes util.TextEncoder and util.TextDecoder available on the global object.

Currently, this is work-in-progress, but I am looking for feedback.

See: #20365

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

This commit makes `util.TextEncoder` and `util.TextDecoder` available on the global object.

AyushG3112 · 2018-05-10T17:48:51Z

test/parallel/test-whatwg-encoding-global.js

+    configurable: true,
+    enumerable: false
+  }
+);


This needs a new line at the end

@AyushG3112 Done: 651ad34

AyushG3112 · 2018-05-10T17:57:12Z

Personally, I don't like the idea of polluting globals. That aside, in this case, I have a particular concern: the names are too ambiguous and potentially confusing to users. TextEncoder does not, in any way signify what it encodes the text to(base64, hex, etc).

I would prefer defining 2 classes on the URL class, and having them being available globally as URL.TextEncoder and URL.TextDecoder.

devsnek · 2018-05-10T18:00:33Z

@AyushG3112 the names and global-ness are part of whatwg spec: https://encoding.spec.whatwg.org

devsnek

code looks good, the docs for them should probably be moved to a separate file though. you might also want to consider moving the whole implementation to a separate file, although I don't know how complex that will make it.

AyushG3112 · 2018-05-10T18:07:14Z

@devsnek I see, thanks for the link!

vsemozhetbyt · 2018-05-10T18:28:09Z

Thank you. doc/api/intl.md also needs updating as well as all posible cross-links inside our docs.

vsemozhetbyt · 2018-05-10T18:34:01Z

Is adding globals semver-major?
Are these classes no more available as the util properties? If so, this is semver-major despite adding globals policy, right?

mscdex · 2018-05-10T19:33:32Z

doc/api/util.md

 <!-- YAML
 added: v8.3.0
+changes:
+  - version: v11.0.0
+    pr-url: https://github.com/nodejs/node/pull/TO_DO


these need updated

mscdex · 2018-05-10T19:34:24Z

doc/api/util.md

 <!-- YAML
 added: v8.3.0
+changes:
+  - version: v11.0.0


I'm not sure this should be specified here, I think we use a placeholder that gets filled in later?

Yes, please use REPLACEME here.

Same for the other v11.0.0 uses in this file and globals.md.

mscdex · 2018-05-10T19:35:13Z

doc/api/util.md

@@ -771,9 +771,13 @@ added: v8.0.0
 * {symbol} that can be used to declare custom promisified variants of functions,
 see [Custom promisified functions][].

-## Class: util.TextDecoder
+## Class: TextDecoder


I think this is confusing if util.TextDecoder is still accessible.

@mscdex I did this for consistency since, in the url docs, the class heading is URL instead of url.URL: https://nodejs.org/dist/latest-v10.x/docs/api/url.html#url_class_url

But I agree with you that this is confusing. In the case of URL, it fits the url docs. In the case of TextDecoder, it doesn't intrinsically fit the util docs.

So I'm gonna change this here, and also follow your suggestion of moving the TextDecoder and TextEncoder docs from util to globals (see: #20662 (comment)).

@mscdex Actually, I'm not quite sure about this. All the globals in globals.md link to other docs, so globals is, in effect, some kind of meta documentation. If I'd move the encoding docs to globals, 90% of the globals documentation would be about encoding, which might be confusing, too. So I don't think is a clear-cut issue.

In fact, I think there are 3 possibilities here:

[1.] Keep the encoding documentation in util and link to it from globals.

pro: Keeps the old documentation structure so people that know the docs already won't be surprised.

con: When searching for "encoding", "text encoding" or similar terms, people are less likely to find the documentation (since the words "globals" and "utilities" are not semantically related to "encoding").

[2.] Move the encoding documentation to globals and link to it from util.

con: The globals docs are currently "meta documentation" in that they only link to other, more detailed documentation. Moving the encoding documentation into this section would break with that.

con: As explained in [1.], when searching for "encoding", less experienced people might not look in the "globals" documentation, making it less likely for them to find the correct documentation.

con (maybe): This could possibly make navigating the globals docs harder in the future, for example, if new globals are introduced that would occur below the lengthy encoding documentation.

[3.] Move the encoding documentation to a new documentation section, e.g. encoding / "Encoding" or text-encoding / "Text Encoding", and link to it from util and globals.

pro: This would perhaps make finding the correct documentation easier for less experienced people.

con (maybe): Documentation-table-of-contents-bloat (if such a thing exists). 😄

In any case, I'm not sure what's a good way to proceed with this. So I'd greatly appreciate more feedback regarding this issue.

PS: When I go to https://nodejs.org/dist/latest-v10.x/docs/api/, I sorely miss a tiny search-box in the top-right corner - I think that would help a lot.

For overall doc search, you can use "View on single page" variant, there is a link at the top of each doc.

@vsemozhetbyt True, but I don't think that's a friendly UX/DX. (Especially on mobile, loading the whole documentation on a single page might not be the best option. But then again, I think this is off-topic here.)

mscdex · 2018-05-10T19:36:12Z

doc/api/globals.md

+
+<!-- type=global -->
+
+The WHATWG `TextEncoder` class. See the [`TextEncoder`][] section.


If what we're doing is migrating from util to a global, I think it'd be better to have the reverse: document here and have util.TextEncoder link to the definition here.

cjihrig

LGTM once all of the comments are addressed.

cjihrig · 2018-05-11T14:26:07Z

doc/api/util.md

 <!-- YAML
 added: v8.3.0
+changes:
+  - version: v11.0.0


Same for the other v11.0.0 uses in this file and globals.md.

Trott · 2018-05-12T18:41:07Z

Is adding globals semver-major?

@vsemozhetbyt I think it is. In my experience, new globals cause failures in some test suites (particularly the one associated with hapijs...not sure if it's code or lab that breaks but one of them, and not sure it actually breaks anymore when a global is added, but it sure used to).

Maybe the thing to do is run CITGM and see if anything breaks.

I'm going to label this semver-major based on the onboarding addendum which says:

if a change has the remote chance of breaking something, go for semver-major

Trott · 2018-05-12T18:41:45Z

@nodejs/tsc Take note. This adds to the global object.

cjihrig · 2018-05-12T22:10:01Z

not sure if it's code or lab that breaks but one of them, and not sure it actually breaks anymore when a global is added

It's lab. lab detects leaked global variables by checking a list of known globals. So, when Node adds a new global, it also requires an update to lab.

Trott · 2018-05-12T23:45:19Z

It's lab. lab detects leaked global variables by checking a list of known globals. So, when Node adds a new global, it also requires an update to lab.

Given that once this changes lands, it will break a whole lot of test suites until the owners update, I think it's definitely semver-major. I'm also adding a notable change label.

jasnell · 2018-05-12T23:49:30Z

Adding a new global is always semver-major

…(comment))

MarkTiedemann · 2018-05-13T13:57:54Z

It's lab. lab detects leaked global variables by checking a list of known globals. So, when Node adds a new global, it also requires an update to lab.

@cjihrig I'm kind of confused about this. Would you mind sending me a link to the lab that you are referring to or giving me a short explanation of what the lab is?

Also, does this have any implications for this PR? Should I make changes to the lab, too? And if so, could you guide me to where these changes are required?

cjihrig · 2018-05-13T14:07:03Z

@cjihrig I'm kind of confused about this. Would you mind sending me a link to the lab that you are referring to or giving me a short explanation of what the lab is?

lab refers to https://www.npmjs.com/package/lab. It's a test runner used by the hapi.js ecosystem. The addition of a new global variable will cause it's global leak detection to fail, which is one reason why new globals have to come in a semver major release.

MarkTiedemann · 2018-05-13T14:44:15Z

@cjihrig Thank you for the explanation!

I created a tiny heads-up PR in lab: hapijs/lab#833 🙂

BridgeAR · 2018-05-13T17:44:41Z

test/common/index.mjs

+
+  if (global.TextDecoder) {
+    knownGlobals.push(TextDecoder)
+  }


If these values are added to the global, they should not be checked for. Instead, they should be added to let knownGlobals = [ ... on top. And I guess common/index.js has to be changed as well?

I agree, they should not be checked for conditionally. So I'm gonna add TextEncoder and TextDecoder to knownGlobals in both test/common/index.mjs and test/common/index.js.

Thanks for the feedback, @BridgeAR!

On another note, I think the recently added URL global should be added to both arrays, too (but I guess in another PR).

BTW, the globals are only added in the following case:

const browserGlobals = !process._noBrowserGlobals; if (browserGlobals) { // ... }

https://github.com/MarkTiedemann/node/blob/5820e161214ba315ab70a1239063bf90f090d6e7/lib/internal/bootstrap/node.js#L73-L74

I could not find any documentation regarding this, except that there was once a --no-browser-globals flag, which doesn't seem to work anymore.

λ node --no-browser-globals node: bad option: --no-browser-globals

So I assume I could remove these 2 lines, too?

EDIT: I found out I can't remove these 2 lines. The no-browser-globals mode can be turned on with configure --no-browser-globals and (...) is not officially supported for regular applications, but still supported after all.

…662 (comment))

TimothyGu · 2018-05-16T15:14:03Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14907/

BridgeAR · 2018-05-18T01:13:51Z

test/common/index.js

-  setTimeout
+  setTimeout,
+  TextDecoder,
+  TextEncoder


I made a mistake in my review earlier. By adding these non-enumerable values we actually do exactly the opposite of what is required in this test. Please remove them and just keep the common files as they were therefore. See #20717 for further details.

@BridgeAR Thanks, will do!

TimothyGu · 2018-05-18T18:12:20Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14964/

MarkTiedemann · 2018-05-21T22:25:32Z

The tests are failing because the globals are not set, as they should be. But I haven't figured out why yet. I'd appreciate any help for fixing this.

- value: undefined,
+ value: [Function: TextEncoder]

MarkTiedemann · 2018-09-17T16:04:28Z

Superseded by #22281.

util: expose the WHATWG Encoding API globally

ffd137a

This commit makes `util.TextEncoder` and `util.TextDecoder` available on the global object.

AyushG3112 reviewed May 10, 2018

View reviewed changes

mscdex added the wip Issues and PRs that are still a work in progress. label May 10, 2018

devsnek approved these changes May 10, 2018

View reviewed changes

vsemozhetbyt added the i18n-api Issues and PRs related to the i18n implementation. label May 10, 2018

mscdex reviewed May 10, 2018

View reviewed changes

jasnell approved these changes May 10, 2018

View reviewed changes

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label May 11, 2018

cjihrig approved these changes May 11, 2018

View reviewed changes

Trott added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels May 12, 2018

Trott added the notable-change PRs with changes that should be highlighted in changelogs. label May 12, 2018

MarkTiedemann added 3 commits May 13, 2018 14:22

Add missing new-line (see: nodejs#20662 (comment))

651ad34

Replace PR-number placeholder (see: nodejs#20662 (comment))

7020995

Replace version number with REPLACEME placeholder (see: nodejs#20662 …

5820e16

…(comment))

MarkTiedemann mentioned this pull request May 13, 2018

Add WHATWG Encoding API globals hapijs/lab#833

Merged

targos mentioned this pull request May 13, 2018

deps: update V8 to 6.7 #19989

Closed

2 tasks

BridgeAR reviewed May 13, 2018

View reviewed changes

Add TextDecoder and TextEncoder to known test globals (see: nodejs#20…

98b1684

…662 (comment))

TimothyGu approved these changes May 16, 2018

View reviewed changes

BridgeAR reviewed May 18, 2018

View reviewed changes

Reverse common test file changes (see: nodejs#20662 (comment))

a6637f0

MarkTiedemann closed this Sep 17, 2018


		<!-- type=global -->

		The WHATWG `TextEncoder` class. See the [`TextEncoder`][] section.

util: expose the WHATWG Encoding API globally #20662

util: expose the WHATWG Encoding API globally #20662

Conversation

MarkTiedemann commented May 10, 2018

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AyushG3112 commented May 10, 2018

devsnek commented May 10, 2018

devsnek left a comment • edited Loading

Choose a reason for hiding this comment

AyushG3112 commented May 10, 2018

vsemozhetbyt commented May 10, 2018

vsemozhetbyt commented May 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkTiedemann May 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented May 12, 2018

Trott commented May 12, 2018

cjihrig commented May 12, 2018

Trott commented May 12, 2018

jasnell commented May 12, 2018

MarkTiedemann commented May 13, 2018

cjihrig commented May 13, 2018

MarkTiedemann commented May 13, 2018

Choose a reason for hiding this comment

MarkTiedemann May 13, 2018 • edited Loading

Choose a reason for hiding this comment

TimothyGu commented May 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented May 18, 2018

MarkTiedemann commented May 21, 2018

MarkTiedemann commented Sep 17, 2018

devsnek left a comment •

edited

Loading

MarkTiedemann May 13, 2018 •

edited

Loading

MarkTiedemann May 13, 2018 •

edited

Loading