util: graduate TextEncoder/TextDecoder, tests by jasnell · Pull Request #15743 · nodejs/node

jasnell · 2017-10-02T21:44:23Z

Add tests ported from Web Platform Tests.

Graduate TextEncoder / TextDecoder from experimental

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

Affected core subsystem(s)

util

mscdex · 2017-10-02T21:48:27Z

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

refack · 2017-10-03T00:06:13Z

test/parallel/test-whatwg-encoding-fatal-streaming.js

Should these be like https://github.com/nodejs/node/blob/c5f243a5f5417af24c10b294d1d57d5dbf64fea3/test/parallel/test-whatwg-url-searchparams-append.js#L8-L13?

No, the URL tests use a completely different format. These had to be ported over to work with out test suite.

Can you put the link to a specific sha instead of blob/master? Otherwise we lose track of the version of the test and if the upstream deletes a file it would be 404

mhdawson · 2017-10-03T20:19:48Z

Not objecting but wondering what the criteria was for deciding it was time to move out of experimental ?

jasnell · 2017-10-03T21:48:00Z

what the criteria for deciding ...

there have been no bugs filed against it and the tests are all coming back good.

jasnell · 2017-10-13T21:20:37Z

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

tniessen · 2017-10-13T21:45:16Z

We might want to handle stability index changes of an API the same as semver-major when it comes to reviews (and yes, I know about the discussions about those rules, but I would like to generally be careful when it comes to exposing APIs.)

jasnell · 2017-10-13T23:12:02Z

I'm -1 on requiring semver-major for bringing things out of experimental. This, for instance, would be a semver-minor addition under any other circumstances, there's really no reason that I can see to force this to a major.

tniessen · 2017-10-14T09:13:58Z

I was not saying we should treat such changes as semver-major, all I was trying to say is that we should review such decisions at least as careful as other public API changes, if not even more. I just referenced semver-major because those are the only changes which get "special treatment" right now.

jasnell · 2017-10-16T16:34:50Z

Ping @nodejs/tsc ... If there are no objections by Wednesday this week I plan to land this.

joyeecheung · 2017-10-17T05:24:37Z

test/parallel/test-whatwg-encoding-fatal-streaming.js

Can you put the link to a specific sha instead of blob/master? Otherwise we lose track of the version of the test and if the upstream deletes a file it would be 404

TimothyGu

Most tests need to be guarded with common.hasIntl.

TimothyGu · 2017-10-17T05:42:31Z

nodejs/build#419

jasnell · 2017-10-17T12:55:36Z

@joyeecheung ... Excellent suggestion
@TimothyGu ... Good catch

jasnell · 2017-10-20T20:20:35Z

@joyeecheung and @TimothyGu ... updated!

jasnell · 2017-10-20T20:22:46Z

linux-nointl CI: https://ci.nodejs.org/job/node-test-commit-linux-nointl/6
Regular CI: https://ci.nodejs.org/job/node-test-pull-request/10877/

TimothyGu · 2017-10-20T22:22:13Z

New nointl CI with #16250 applied: https://ci.nodejs.org/job/node-test-commit-linux-nointl/7

TimothyGu · 2017-10-20T22:23:15Z

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

This test shouldn't be skipped.

TimothyGu · 2017-10-20T22:23:57Z

test/parallel/test-whatwg-encoding-surrogates-utf8.js

This test should work even when ICU is not enabled.

TimothyGu · 2017-10-20T22:24:34Z

test/parallel/test-whatwg-encoding-textdecoder-ignorebom.js

With the exception of UTF-16BE, this test should work even when ICU is not enabled.

TimothyGu · 2017-10-20T22:25:02Z

test/parallel/test-whatwg-encoding-textdecoder-streaming.js

With the exception of UTF-16BE, this test should work even when ICU is not enabled.

TimothyGu · 2017-10-20T22:25:59Z

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

With the exception of UTF-16BE, this test should work even when ICU is not enabled.

TimothyGu · 2017-10-20T22:26:21Z

test/parallel/test-whatwg-encoding-textencoder-utf16-surrogates.js

This test should work even when ICU is not enabled.

TimothyGu · 2017-10-20T22:27:19Z

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

This test should work even when ICU is not enabled.

joyeecheung

One suggestion, otherwise LGTM with @TimothyGu 's review addressed.

joyeecheung · 2017-10-21T11:58:35Z

test/parallel/test-whatwg-encoding-fatal-streaming.js

We can use the first 7 characters of the SHA if we don't want a really long URL here. That works as well.

I usually prefer 10 characters, as that's what modern Git outputs IIRC.

Oh, right, forgot about those :-)

joyeecheung · 2017-10-21T12:07:17Z

There is a compilation error

In file included from ../deps/v8/src/setup-isolate-full.cc:8:
In file included from ../deps/v8/src/interpreter/interpreter.h:14:
In file included from ../deps/v8/src/builtins/builtins.h:8:
In file included from ../deps/v8/src/base/flags.h:8:
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.9/../../../../include/c++/4.9/cstddef:51:11: error: no member named 'max_align_t' in the global namespace
  using ::max_align_t;
        ~~^
1 error generated.

Looks like duplicate of nodejs/build#685 cc @nodejs/build
EDIT: looks like the support for that combination is removed anyway (nodejs/build#688), so probably we just need to remove that bot(ubuntu1204-clang341-64) or skip it when targeting branches newer than 8.

gibfahn · 2017-10-21T13:01:54Z

EDIT: looks like the support for that combination is removed anyway (nodejs/build#688), so probably we just need to remove that bot(ubuntu1204-clang341-64) or skip it when targeting branches newer than 8.

I've removed it, AIUI the need is for a run to make sure no-intl works, there's no need to have full coverage. If anyone disagrees feel free to chime in on nodejs/build#419.

Add tests ported from Web Platform Tests. Graduate TextEncoder / TextDecoder from experimental

Add tests ported from Web Platform Tests. Graduate TextEncoder / TextDecoder from experimental PR-URL: #15743 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

jasnell · 2017-10-24T04:22:51Z

Landed in 7f9eb4c with some lints fixed.

Add tests ported from Web Platform Tests. Graduate TextEncoder / TextDecoder from experimental PR-URL: nodejs/node#15743 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

Add tests ported from Web Platform Tests. Graduate TextEncoder / TextDecoder from experimental PR-URL: #15743 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

This LTS release comes with 87 commits. This includes 30 that are updates to lib/ or src/, 20 that are test related, 13 that are doc related, 19 which are build / tools related, and 4 commits which are updates to dependencies. Notable Changes: * doc: - add Gibson Fahnestock to Release team (Gibson Fahnestock) #16620 * deps: - update npm to 5.5.1 (Myles Borins) #16509 * http2: - The exposed http2 socket is no longer manipulatable (Anatoli Papirovski) #16330 * module: - support custom paths to require.resolve() (cjihrig) #16397 * util: - util.TextEncoder and util.TextDecoder are no longer experimental. There will no longer be a warning when they are used (James M Snell) #15743 PR-URL: #16630

This LTS release comes with 87 commits. This includes 30 that are updates to lib/ or src/, 20 that are test related, 13 that are doc related, 19 which are build / tools related, and 4 commits which are updates to dependencies. Notable Changes: * doc: - add Gibson Fahnestock to Release team (Gibson Fahnestock) nodejs/node#16620 * deps: - update npm to 5.5.1 (Myles Borins) nodejs/node#16509 * http2: - The exposed http2 socket is no longer manipulatable (Anatoli Papirovski) nodejs/node#16330 * module: - support custom paths to require.resolve() (cjihrig) nodejs/node#16397 * util: - util.TextEncoder and util.TextDecoder are no longer experimental. There will no longer be a warning when they are used (James M Snell) nodejs/node#15743 PR-URL: nodejs/node#16630

Add tests ported from Web Platform Tests. Graduate TextEncoder / TextDecoder from experimental PR-URL: nodejs/node#15743 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

This LTS release comes with 87 commits. This includes 30 that are updates to lib/ or src/, 20 that are test related, 13 that are doc related, 19 which are build / tools related, and 4 commits which are updates to dependencies. Notable Changes: * doc: - add Gibson Fahnestock to Release team (Gibson Fahnestock) nodejs/node#16620 * deps: - update npm to 5.5.1 (Myles Borins) nodejs/node#16509 * http2: - The exposed http2 socket is no longer manipulatable (Anatoli Papirovski) nodejs/node#16330 * module: - support custom paths to require.resolve() (cjihrig) nodejs/node#16397 * util: - util.TextEncoder and util.TextDecoder are no longer experimental. There will no longer be a warning when they are used (James M Snell) nodejs/node#15743 PR-URL: nodejs/node#16630

gibfahn · 2018-01-15T22:33:04Z

Release team were -1 on landing on v6.x, if you disagree let us know.

nodejs-github-bot added the encoding Issues and PRs related to the TextEncoder and TextDecoder APIs. label Oct 2, 2017

refack reviewed Oct 3, 2017

View reviewed changes

cjihrig approved these changes Oct 3, 2017

View reviewed changes

refack approved these changes Oct 3, 2017

View reviewed changes

jasnell added notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. labels Oct 13, 2017

TimothyGu self-requested a review October 14, 2017 04:09

TimothyGu mentioned this pull request Oct 15, 2017

Import TextDecoder/TextEncoder Web Platform Tests #15442

Closed

addaleax approved these changes Oct 16, 2017

View reviewed changes

joyeecheung requested changes Oct 17, 2017

View reviewed changes

TimothyGu suggested changes Oct 17, 2017

View reviewed changes

jasnell force-pushed the graduate-encoding branch from c5f243a to 56b1cb6 Compare October 20, 2017 19:23

TimothyGu suggested changes Oct 20, 2017

View reviewed changes

joyeecheung approved these changes Oct 21, 2017

View reviewed changes

util: graduate TextEncoder/TextDecoder, tests

8792273

Add tests ported from Web Platform Tests. Graduate TextEncoder / TextDecoder from experimental

jasnell added 4 commits October 23, 2017 21:02

[Squash]

52b6b94

[Squash]

45f7c79

[Squash]

dcbcfa3

[Squash]

795bb3e

jasnell closed this Oct 24, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

gibfahn mentioned this pull request Oct 31, 2017

v8.9.0 proposal #16630

Merged

schmod mentioned this pull request Nov 2, 2017

Proposal: Initial "Active LTS" release should follow LTS policies nodejs/Release#281

Closed

gibfahn added dont-land-on-v6.x labels Jan 15, 2018

Uh oh!

Conversation

jasnell commented Oct 2, 2017

Checklist

Affected core subsystem(s)

Uh oh!

mscdex commented Oct 2, 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!

mhdawson commented Oct 3, 2017

Uh oh!

jasnell commented Oct 3, 2017

Uh oh!

jasnell commented Oct 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tniessen commented Oct 13, 2017

Uh oh!

jasnell commented Oct 13, 2017

Uh oh!

tniessen commented Oct 14, 2017

Uh oh!

jasnell commented Oct 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Oct 17, 2017

Uh oh!

jasnell commented Oct 17, 2017

Uh oh!

jasnell commented Oct 20, 2017

Uh oh!

jasnell commented Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimothyGu commented Oct 20, 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!

joyeecheung left a comment

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!

joyeecheung commented Oct 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gibfahn commented Oct 21, 2017

Uh oh!

jasnell commented Oct 24, 2017

Uh oh!

gibfahn commented Jan 15, 2018

Uh oh!

Reviewers

Assignees

Labels

jasnell commented Oct 13, 2017 •

edited

Loading

jasnell commented Oct 20, 2017 •

edited

Loading

joyeecheung commented Oct 21, 2017 •

edited

Loading