Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

util: expose the WHATWG Encoding API globally #20662

Closed
wants to merge 6 commits into from
Closed

util: expose the WHATWG Encoding API globally #20662

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ffd137a
util: expose the WHATWG Encoding API globally
MarkTiedemann May 10, 2018
651ad34
Add missing new-line (see: https://github.com/nodejs/node/pull/20662#…
MarkTiedemann May 13, 2018
7020995
Replace PR-number placeholder (see: https://github.com/nodejs/node/pu…
MarkTiedemann May 13, 2018
5820e16
Replace version number with REPLACEME placeholder (see: https://githu…
MarkTiedemann May 13, 2018
98b1684
Add TextDecoder and TextEncoder to known test globals (see: https://g…
MarkTiedemann May 13, 2018
a6637f0
Reverse common test file changes (see: https://github.com/nodejs/node…
MarkTiedemann May 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions doc/api/errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -764,13 +764,13 @@ The stack trace is extended to include the point in time at which the
<a id="ERR_ENCODING_INVALID_ENCODED_DATA"></a>
### ERR_ENCODING_INVALID_ENCODED_DATA

Data provided to `util.TextDecoder()` API was invalid according to the encoding
Data provided to `TextDecoder()` API was invalid according to the encoding
provided.

<a id="ERR_ENCODING_NOT_SUPPORTED"></a>
### ERR_ENCODING_NOT_SUPPORTED

Encoding provided to `util.TextDecoder()` API was not one of the
Encoding provided to `TextDecoder()` API was not one of the
[WHATWG Supported Encodings][].

<a id="ERR_FALSY_VALUE_REJECTION"></a>
Expand Down
20 changes: 20 additions & 0 deletions doc/api/globals.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,24 @@ added: v10.0.0

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

## TextEncoder
<!-- YAML
added: REPLACEME
-->

<!-- type=global -->

The WHATWG `TextEncoder` class. See the [`TextEncoder`][] section.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


## TextDecoder
<!-- YAML
added: REPLACEME
-->

<!-- type=global -->

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

[`__dirname`]: modules.html#modules_dirname
[`__filename`]: modules.html#modules_filename
[`clearImmediate`]: timers.html#timers_clearimmediate_immediate
Expand All @@ -171,6 +189,8 @@ The WHATWG `URLSearchParams` class. See the [`URLSearchParams`][] section.
[`setTimeout`]: timers.html#timers_settimeout_callback_delay_args
[`URL`]: url.html#url_class_url
[`URLSearchParams`]: url.html#url_class_urlsearchparams
[`TextEncoder`]: util.html#util_class_textencoder
[`TextDecoder`]: util.html#util_class_textdecoder
[buffer section]: buffer.html
[built-in objects]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects
[module system documentation]: modules.html
Expand Down
12 changes: 10 additions & 2 deletions doc/api/util.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@MarkTiedemann MarkTiedemann May 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@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.)

<!-- YAML
added: v8.3.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/20662
description: The class is now available on the global object.
-->

An implementation of the [WHATWG Encoding Standard][] `TextDecoder` API.
Expand Down Expand Up @@ -908,9 +912,13 @@ thrown.
The value will be `true` if the decoding result will include the byte order
mark.

## Class: util.TextEncoder
## Class: TextEncoder
<!-- YAML
added: v8.3.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/20662
description: The class is now available on the global object.
-->

An implementation of the [WHATWG Encoding Standard][] `TextEncoder` API. All
Expand Down
19 changes: 19 additions & 0 deletions lib/internal/bootstrap/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@
setupGlobalTimeouts();
setupGlobalConsole();
setupGlobalURL();
setupGlobalTextEncoderDecoder();
}

// Ensure setURLConstructor() is called before the native
Expand Down Expand Up @@ -381,6 +382,24 @@
});
}

function setupGlobalTextEncoderDecoder() {
const { TextEncoder, TextDecoder } = NativeModule.require('internal/util');
Object.defineProperties(global, {
TextEncoder: {
value: TextEncoder,
writable: true,
configurable: true,
enumerable: false
},
TextDecoder: {
value: TextDecoder,
writable: true,
configurable: true,
enumerable: false
}
});
}

function setupInspector(originalConsole, wrappedConsole, CJSModule) {
if (!process.config.variables.v8_enable_inspector) {
return;
Expand Down
9 changes: 9 additions & 0 deletions test/common/index.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,15 @@ export function leakedGlobals() {
knownGlobals.push(Symbol);
}

// WHATWG APIs
if (global.TextEncoder) {
knownGlobals.push(TextEncoder);
}

if (global.TextDecoder) {
knownGlobals.push(TextDecoder)
}
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@MarkTiedemann MarkTiedemann May 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.


const leaked = [];

for (const val in global) {
Expand Down
25 changes: 25 additions & 0 deletions test/parallel/test-whatwg-encoding-global.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
'use strict';

require('../common');
const assert = require('assert');
const { TextEncoder, TextDecoder } = require('util');

assert.deepStrictEqual(
Object.getOwnPropertyDescriptor(global, 'TextEncoder'),
{
value: TextEncoder,
writable: true,
configurable: true,
enumerable: false
}
);

assert.deepStrictEqual(
Object.getOwnPropertyDescriptor(global, 'TextDecoder'),
{
value: TextDecoder,
writable: true,
configurable: true,
enumerable: false
}
);