Skip to content

util: make TextEncoder/TextDecoder global - #22281

Closed
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:global-textencoder-textdecoder
Closed

util: make TextEncoder/TextDecoder global#22281
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:global-textencoder-textdecoder

Conversation

@jasnell

@jasnelljasnell commented Aug 12, 2018

Copy link
Copy Markdown
Member

Fixes: #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

@jasnelljasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 12, 2018
@nodejs-github-bot

nodejs-github-bot commented Aug 12, 2018

Copy link
Copy Markdown
Collaborator

@jasnell build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/532/pipeline

@vsemozhetbytvsemozhetbyt added i18n-api Issues and PRs related to the i18n implementation. encoding Issues and PRs related to the TextEncoder and TextDecoder APIs. labels Aug 12, 2018
vsemozhetbyt
vsemozhetbyt reviewed Aug 12, 2018
View reviewed changes
Comment threaddoc/api/globals.md Outdated

@vsemozhetbytvsemozhetbytAug 12, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Both sections need to be placed before ## URL section, ABC-wise.

Comment threaddoc/api/globals.md Outdated

@vsemozhetbytvsemozhetbytAug 12, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Misplaced part of the ## URLSearchParams section.

@vsemozhetbyt

vsemozhetbyt commented Aug 12, 2018

Copy link
Copy Markdown
Contributor

It seems intl.md also needs updating, see require('util').TextDecoder mentions.

@jasnell

jasnell commented Aug 12, 2018

Copy link
Copy Markdown
MemberAuthor

@vsemozhetbyt ... wasn't sure if we should update intl.md to be honest.

TimothyGu
TimothyGu approved these changes Aug 12, 2018
View reviewed changes

@TimothyGuTimothyGu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM with one nit

Comment threadlib/internal/bootstrap/node.js Outdated

@TimothyGuTimothyGuAug 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Per Web IDL the properties should not be enumerable.

@jasnelljasnellAug 15, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

ah right... I knew something was off about that. Thx!

@BridgeARBridgeAR added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Aug 13, 2018
@BridgeAR
BridgeAR requested a review from a teamAugust 13, 2018 00:32
@BridgeARBridgeAR removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Aug 13, 2018
BridgeAR
BridgeAR approved these changes Aug 13, 2018
View reviewed changes

@BridgeARBridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM with the comments addressed.

Comment threadtest/common/index.js Outdated

@BridgeARBridgeARAug 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This has to be removed again as soon as they are not enumerable anymore.

@jasnell

jasnell commented Aug 15, 2018

Copy link
Copy Markdown
MemberAuthor

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

@jasnell
jasnellforce-pushed the global-textencoder-textdecoder branch from 02c6a27 to 0e437e1CompareAugust 15, 2018 22:49
mcollina
mcollina reviewed Aug 16, 2018
View reviewed changes

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you please retain a test that check that what is exposed through util is the same of the globals?

@joyeecheungjoyeecheung mentioned this pull request Aug 17, 2018
Closed
@jasnell

jasnell commented Sep 10, 2018

Copy link
Copy Markdown
MemberAuthor

Will be updating this PR this week.

@jasnell
jasnellforce-pushed the global-textencoder-textdecoder branch from 0e437e1 to 75f1128CompareSeptember 12, 2018 14:34
@jasnell

jasnell commented Sep 12, 2018

Copy link
Copy Markdown
MemberAuthor

@mcollina ... requested test added!

@jasnell

jasnell commented Sep 12, 2018

Copy link
Copy Markdown
MemberAuthor

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

@mcollina

mcollina commented Sep 12, 2018

Copy link
Copy Markdown
Member

Note that this would likely break lab and possibly some other testing frameworks, cc @geek.

mcollina
mcollina approved these changes Sep 12, 2018
View reviewed changes

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@trivikr

trivikr commented Sep 15, 2018•
edited
Loading

Copy link
Copy Markdown
Member

Resumed CI: https://ci.nodejs.org/job/node-test-pull-request/17204/
Resumed CI: https://ci.nodejs.org/job/node-test-pull-request/17209/
Resume CI: https://ci.nodejs.org/job/node-test-pull-request/17223/

@mcollinamcollina added the needs-ci PRs that need a full CI run. label Sep 17, 2018
@mcollina

mcollina commented Sep 17, 2018

Copy link
Copy Markdown
Member

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

@addaleaxaddaleax removed the needs-ci PRs that need a full CI run. label Sep 17, 2018
@addaleax

addaleax commented Sep 17, 2018

Copy link
Copy Markdown
Member

Landed in 932be01

addaleax pushed a commit that referenced this pull request Sep 17, 2018
@jasnell@addaleax
util: make TextEncoder/TextDecoder global
932be01
Fixes: #20365
PR-URL: #22281
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@MarkTiedemannMarkTiedemann mentioned this pull request Sep 17, 2018
Closed
4 tasks
jasnell added a commit that referenced this pull request Oct 2, 2018
@jasnell
2018-10-23, Version 11.0.0 (Current)
6424d85
Notable changes:
* Build
* FreeBSD 10 is no longer supported. [#22617](#22617)
* `child_process`
* The default value of the `windowsHide` option has been changed to `true`. [#21316](#21316)
* `console`
* `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](#21649)
* `console.time()` will no longer reset a timer if it already exists. [#20442](#20442)
* `crypto`
* PEM-level encryption is now supported. [#23151](#23151)
* An API for key pair generation has been added. [#22660](#22660)
* Dependencies
* V8 has been updated to 7.0. [#22754](#22754)
* `fs`
* The `fs.read()` method now requires a callback. [#22146](#22146)
* The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](#20735)
* `http`
* The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](#20270)
* `http2`
* An event will be emitted when a `PING` frame is received. [#23009](#23009)
* Support for the `ORIGIN` frame has been added. [#22956](#22956)
* General
* Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating.
* An experimental implementation of `queueMicrotask()` has been added. [#22951](#22951)
* Internal
* Windows performance-counter support has been removed. [#22485](#22485)
* The `--expose-http2` command-line option has been removed. [#20887](#20887)
* Promises
* A new `multipleResolves` event will be emitted when a Promise is resolved (or rejected) more than once. [#22218](#22218)
* Timers
* Interval timers will be rescheduled even if previous interval threw an error. [#20002](#20002)
* `util`
* The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](#22281)
* `util.inspect()` output size is limited to 128 MB by default. [#22756](#22756)
* A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](#21914)
@jasnelljasnell mentioned this pull request Oct 2, 2018
Closed
4 tasks
jasnell added a commit that referenced this pull request Oct 17, 2018
@jasnell
2018-10-23, Version 11.0.0 (Current)
839823a
Notable changes:
* Build
* FreeBSD 10 is no longer supported.[#22617](#22617)
* `child_process`
* The default value of the `windowsHide` option has been changed
to `true`. [#21316](#21316)
* `console`
* `console.countReset()` will emit a warning if the timer
being reset does not exist. [#21649](#21649)
* `console.time()` will no longer reset a timer if it already
exists. [#20442](#20442)
* Dependencies
* V8 has been updated to 7.0.
[#22754](#22754)
* `fs`
* The `fs.read()` method now requires a callback.
[#22146](#22146)
* The previously deprecated `fs.SyncWriteStream` utility has been
removed.[#20735](#20735)
* `http`
* The `http`, `https`, and `tls` modules now use the WHATWG URL parser
by default. [#20270](#20270)
* General
* Use of `process.binding()` has been deprecated. Userland code using
`process.binding()` should re-evaluate that use and begin migrating. If
there are no supported API alternatives, please open an issue in the
Node.js GitHub repository so that a suitable alternative may be discussed.
* An experimental implementation of `queueMicrotask()` has been added.
[#22951](#22951)
* Internal
* Windows performance-counter support has been removed.
[#22485](#22485)
* The `--expose-http2` command-line option has been removed.
[#20887](#20887)
* Timers
* Interval timers will be rescheduled even if previous interval threw
an error. [#20002](#20002)
* `util`
* The WHATWG `TextEncoder` and `TextDecoder` are now globals.
[#22281](#22281)
* `util.inspect()` output size is limited to 128 MB by default.
[#22756](#22756)
* A runtime warning will be emitted when `NODE_DEBUG` is set for
either `http` or `http2`. [#21914](#21914)
jasnell added a commit that referenced this pull request Oct 17, 2018
@jasnell
2018-10-23, Version 11.0.0 (Current)
c8d89c9
Notable changes:
* Build
* FreeBSD 10 is no longer supported.[#22617](#22617)
* `child_process`
* The default value of the `windowsHide` option has been changed
to `true`. [#21316](#21316)
* `console`
* `console.countReset()` will emit a warning if the timer
being reset does not exist. [#21649](#21649)
* `console.time()` will no longer reset a timer if it already
exists. [#20442](#20442)
* Dependencies
* V8 has been updated to 7.0.
[#22754](#22754)
* `fs`
* The `fs.read()` method now requires a callback.
[#22146](#22146)
* The previously deprecated `fs.SyncWriteStream` utility has been
removed.[#20735](#20735)
* `http`
* The `http`, `https`, and `tls` modules now use the WHATWG URL parser
by default. [#20270](#20270)
* General
* Use of `process.binding()` has been deprecated. Userland code using
`process.binding()` should re-evaluate that use and begin migrating. If
there are no supported API alternatives, please open an issue in the
Node.js GitHub repository so that a suitable alternative may be discussed.
* An experimental implementation of `queueMicrotask()` has been added.
[#22951](#22951)
* Internal
* Windows performance-counter support has been removed.
[#22485](#22485)
* The `--expose-http2` command-line option has been removed.
[#20887](#20887)
* Timers
* Interval timers will be rescheduled even if previous interval threw
an error. [#20002](#20002)
* `util`
* The WHATWG `TextEncoder` and `TextDecoder` are now globals.
[#22281](#22281)
* `util.inspect()` output size is limited to 128 MB by default.
[#22756](#22756)
* A runtime warning will be emitted when `NODE_DEBUG` is set for
either `http` or `http2`. [#21914](#21914)
jasnell added a commit that referenced this pull request Oct 21, 2018
@jasnell
2018-10-23, Version 11.0.0 (Current)
ab20ac4
Notable changes:
* Build
* FreeBSD 10 is no longer supported.[#22617](#22617)
* `child_process`
* The default value of the `windowsHide` option has been changed
to `true`. [#21316](#21316)
* `console`
* `console.countReset()` will emit a warning if the timer
being reset does not exist. [#21649](#21649)
* `console.time()` will no longer reset a timer if it already
exists. [#20442](#20442)
* Dependencies
* V8 has been updated to 7.0.
[#22754](#22754)
* `fs`
* The `fs.read()` method now requires a callback.
[#22146](#22146)
* The previously deprecated `fs.SyncWriteStream` utility has been
removed.[#20735](#20735)
* `http`
* The `http`, `https`, and `tls` modules now use the WHATWG URL parser
by default. [#20270](#20270)
* General
* Use of `process.binding()` has been deprecated. Userland code using
`process.binding()` should re-evaluate that use and begin migrating. If
there are no supported API alternatives, please open an issue in the
Node.js GitHub repository so that a suitable alternative may be discussed.
* An experimental implementation of `queueMicrotask()` has been added.
[#22951](#22951)
* Internal
* Windows performance-counter support has been removed.
[#22485](#22485)
* The `--expose-http2` command-line option has been removed.
[#20887](#20887)
* Timers
* Interval timers will be rescheduled even if previous interval threw
an error. [#20002](#20002)
* `util`
* The WHATWG `TextEncoder` and `TextDecoder` are now globals.
[#22281](#22281)
* `util.inspect()` output size is limited to 128 MB by default.
[#22756](#22756)
* A runtime warning will be emitted when `NODE_DEBUG` is set for
either `http` or `http2`. [#21914](#21914)
jasnell added a commit that referenced this pull request Oct 22, 2018
@jasnell
2018-10-23, Version 11.0.0 (Current)
cea049b
Notable changes:
* Build
* FreeBSD 10 is no longer supported.[#22617](#22617)
* `child_process`
* The default value of the `windowsHide` option has been changed
to `true`. [#21316](#21316)
* `console`
* `console.countReset()` will emit a warning if the timer
being reset does not exist. [#21649](#21649)
* `console.time()` will no longer reset a timer if it already
exists. [#20442](#20442)
* Dependencies
* V8 has been updated to 7.0.
[#22754](#22754)
* `fs`
* The `fs.read()` method now requires a callback.
[#22146](#22146)
* The previously deprecated `fs.SyncWriteStream` utility has been
removed.[#20735](#20735)
* `http`
* The `http`, `https`, and `tls` modules now use the WHATWG URL parser
by default. [#20270](#20270)
* General
* Use of `process.binding()` has been deprecated. Userland code using
`process.binding()` should re-evaluate that use and begin migrating. If
there are no supported API alternatives, please open an issue in the
Node.js GitHub repository so that a suitable alternative may be discussed.
* An experimental implementation of `queueMicrotask()` has been added.
[#22951](#22951)
* Internal
* Windows performance-counter support has been removed.
[#22485](#22485)
* The `--expose-http2` command-line option has been removed.
[#20887](#20887)
* Timers
* Interval timers will be rescheduled even if previous interval threw
an error. [#20002](#20002)
* `util`
* The WHATWG `TextEncoder` and `TextDecoder` are now globals.
[#22281](#22281)
* `util.inspect()` output size is limited to 128 MB by default.
[#22756](#22756)
* A runtime warning will be emitted when `NODE_DEBUG` is set for
either `http` or `http2`. [#21914](#21914)
devsnek pushed a commit to devsnek/node that referenced this pull request Oct 23, 2018
@jasnell
2018-10-23, Version 11.0.0 (Current)
3516052
Notable changes:
* Build
* FreeBSD 10 is no longer supported.[nodejs#22617](nodejs#22617)
* `child_process`
* The default value of the `windowsHide` option has been changed
to `true`. [nodejs#21316](nodejs#21316)
* `console`
* `console.countReset()` will emit a warning if the timer
being reset does not exist. [nodejs#21649](nodejs#21649)
* `console.time()` will no longer reset a timer if it already
exists. [nodejs#20442](nodejs#20442)
* Dependencies
* V8 has been updated to 7.0.
[nodejs#22754](nodejs#22754)
* `fs`
* The `fs.read()` method now requires a callback.
[nodejs#22146](nodejs#22146)
* The previously deprecated `fs.SyncWriteStream` utility has been
removed.[nodejs#20735](nodejs#20735)
* `http`
* The `http`, `https`, and `tls` modules now use the WHATWG URL parser
by default. [nodejs#20270](nodejs#20270)
* General
* Use of `process.binding()` has been deprecated. Userland code using
`process.binding()` should re-evaluate that use and begin migrating. If
there are no supported API alternatives, please open an issue in the
Node.js GitHub repository so that a suitable alternative may be discussed.
* An experimental implementation of `queueMicrotask()` has been added.
[nodejs#22951](nodejs#22951)
* Internal
* Windows performance-counter support has been removed.
[nodejs#22485](nodejs#22485)
* The `--expose-http2` command-line option has been removed.
[nodejs#20887](nodejs#20887)
* Timers
* Interval timers will be rescheduled even if previous interval threw
an error. [nodejs#20002](nodejs#20002)
* `util`
* The WHATWG `TextEncoder` and `TextDecoder` are now globals.
[nodejs#22281](nodejs#22281)
* `util.inspect()` output size is limited to 128 MB by default.
[nodejs#22756](nodejs#22756)
* A runtime warning will be emitted when `NODE_DEBUG` is set for
either `http` or `http2`. [nodejs#21914](nodejs#21914)
@okofishokofish mentioned this pull request Nov 2, 2018
Merged
deepak1556 pushed a commit to electron/node that referenced this pull request Dec 10, 2018
@jasnell@deepak1556
2018-10-23, Version 11.0.0 (Current)
fffd852
Notable changes:
* Build
* FreeBSD 10 is no longer supported.[#22617](nodejs/node#22617)
* `child_process`
* The default value of the `windowsHide` option has been changed
to `true`. [#21316](nodejs/node#21316)
* `console`
* `console.countReset()` will emit a warning if the timer
being reset does not exist. [#21649](nodejs/node#21649)
* `console.time()` will no longer reset a timer if it already
exists. [#20442](nodejs/node#20442)
* Dependencies
* V8 has been updated to 7.0.
[#22754](nodejs/node#22754)
* `fs`
* The `fs.read()` method now requires a callback.
[#22146](nodejs/node#22146)
* The previously deprecated `fs.SyncWriteStream` utility has been
removed.[#20735](nodejs/node#20735)
* `http`
* The `http`, `https`, and `tls` modules now use the WHATWG URL parser
by default. [#20270](nodejs/node#20270)
* General
* Use of `process.binding()` has been deprecated. Userland code using
`process.binding()` should re-evaluate that use and begin migrating. If
there are no supported API alternatives, please open an issue in the
Node.js GitHub repository so that a suitable alternative may be discussed.
* An experimental implementation of `queueMicrotask()` has been added.
[#22951](nodejs/node#22951)
* Internal
* Windows performance-counter support has been removed.
[#22485](nodejs/node#22485)
* The `--expose-http2` command-line option has been removed.
[#20887](nodejs/node#20887)
* Timers
* Interval timers will be rescheduled even if previous interval threw
an error. [#20002](nodejs/node#20002)
* `util`
* The WHATWG `TextEncoder` and `TextDecoder` are now globals.
[#22281](nodejs/node#22281)
* `util.inspect()` output size is limited to 128 MB by default.
[#22756](nodejs/node#22756)
* A runtime warning will be emitted when `NODE_DEBUG` is set for
either `http` or `http2`. [#21914](nodejs/node#21914)
deepak1556 pushed a commit to electron/node that referenced this pull request Dec 19, 2018
@jasnell@deepak1556
2018-10-23, Version 11.0.0 (Current)
0b2f9b4
Notable changes:
* Build
* FreeBSD 10 is no longer supported.[#22617](nodejs/node#22617)
* `child_process`
* The default value of the `windowsHide` option has been changed
to `true`. [#21316](nodejs/node#21316)
* `console`
* `console.countReset()` will emit a warning if the timer
being reset does not exist. [#21649](nodejs/node#21649)
* `console.time()` will no longer reset a timer if it already
exists. [#20442](nodejs/node#20442)
* Dependencies
* V8 has been updated to 7.0.
[#22754](nodejs/node#22754)
* `fs`
* The `fs.read()` method now requires a callback.
[#22146](nodejs/node#22146)
* The previously deprecated `fs.SyncWriteStream` utility has been
removed.[#20735](nodejs/node#20735)
* `http`
* The `http`, `https`, and `tls` modules now use the WHATWG URL parser
by default. [#20270](nodejs/node#20270)
* General
* Use of `process.binding()` has been deprecated. Userland code using
`process.binding()` should re-evaluate that use and begin migrating. If
there are no supported API alternatives, please open an issue in the
Node.js GitHub repository so that a suitable alternative may be discussed.
* An experimental implementation of `queueMicrotask()` has been added.
[#22951](nodejs/node#22951)
* Internal
* Windows performance-counter support has been removed.
[#22485](nodejs/node#22485)
* The `--expose-http2` command-line option has been removed.
[#20887](nodejs/node#20887)
* Timers
* Interval timers will be rescheduled even if previous interval threw
an error. [#20002](nodejs/node#20002)
* `util`
* The WHATWG `TextEncoder` and `TextDecoder` are now globals.
[#22281](nodejs/node#22281)
* `util.inspect()` output size is limited to 128 MB by default.
[#22756](nodejs/node#22756)
* A runtime warning will be emitted when `NODE_DEBUG` is set for
either `http` or `http2`. [#21914](nodejs/node#21914)
@coderabbitaicoderabbitaiBot mentioned this pull request Jun 1, 2026
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes
@cjihrigcjihrigcjihrig approved these changes
@BridgeARBridgeARBridgeAR approved these changes
+4 more reviewers
@vsemozhetbytvsemozhetbytvsemozhetbyt left review comments
@thefourtheyethefourtheyethefourtheye approved these changes
@TimothyGuTimothyGuTimothyGu approved these changes
@devsnekdevsnekdevsnek approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

encodingIssues and PRs related to the TextEncoder and TextDecoder APIs.i18n-apiIssues and PRs related to the i18n implementation.semver-majorPRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

11 participants

@jasnell@nodejs-github-bot@vsemozhetbyt@mcollina@trivikr@addaleax@thefourtheye@TimothyGu@cjihrig@devsnek@BridgeAR