Fix building without ICU by TimothyGu · Pull Request #14489 · nodejs/node · GitHub

Corresponding documentation changes for encoding will be folded into #14486.

Checklist

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

Affected core subsystem(s)

encoding, test

Argh... we seriously need to get building without ICU enabled by default as part of the normal CI run.

LGTM. Only lightly reviewed, mostly rubber-stamp.

@jasnell That's nodejs/build#419.

@jasnell I assume your comment was a LGTM?

I modified this PR to offer a TextDecoder with reduced functionalities even when built without ICU. @jasnell @bnoordhuis please review.

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

s/options/option

This change does not look related?

This is necessary to fix building (and testing) on ICU, since the JS implementation can only distinguish between wide/narrow.

Code change looks good, tho I'd prefer a bit less code duplication on the two TextDecoder variants.

Looks good.
I've hit that "ICU missing" wall several times... good riddance.

Don't you want to assert anything about the output string? Or at least wrap in assert.doesNotThrow just to be explicit.

Wrapped in doesNotThrow. We don't really care about the inspection results as long as it works 😉

Could you add the Type of the expected Error.

Same comments as in test-whatwg-encoding-textencoder.js#L24 & L28

You could use a simple string, should be semantically the same as an anchored RegExp.

Could you add a test asserting the expected message, to test/parallel/test-internal-errors.js, as per the new tweaks in https://github.com/nodejs/node/blob/master/doc/guides/using-internal-errors.md#testing-new-errors

Thanks for the reminder, I realized that I also forgot to add the new error code to the docs...

Actually, the test file only contains more complicated error code processing, like ERR_INVALID_ARG_TYPE. I'll still add the missing docs though.

The reasoning was "test the message once then assume it's ok everywhere else" but, this will probably get reworked anyway after we finish migrating all the Error, so this is non-blocking.

obsolete

@jasnell Changed so that there's less duplication.

Tried to kick off a CI w/o ICU: https://ci.nodejs.org/job/node-test-commit/11502/ https://ci.nodejs.org/job/node-test-commit/11503/

Also split up the tests.

After the V8 6.0 update, the V8 console is now installed on the global object regardless of whether inspector is actually enabled, and in the case of no-ICU inspector is always disabled, which leads to the test failures :(

Pushed one more fix to address this situation.

New no ICU CI: https://ci.nodejs.org/job/node-test-commit/11505/

Landed in 34d1b11...17547c4.

PR-URL: #14489 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Also split up the tests. PR-URL: #14489 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

PR-URL: #14489 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

This doesn’t land cleanly on 8.x; if you can, please follow the guide and raise a backport PR.

Note that the resulting package does not work yet due to nodejs/node#14489 not being in 8.3.0.

Also split up the tests. PR-URL: nodejs#14489 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

@addaleax Done: #14786. Only the second commit needs backporting, as the first and third commits have been backported already.

Also split up the tests. Backport-PR-URL: #14786 Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net> PR-URL: #14489 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Is this needed on v6.x?

ping. this would need to be manually backported

TimothyGu added encoding Issues and PRs related to the TextEncoder and TextDecoder APIs. test Issues and PRs related to the tests. labels Jul 26, 2017

TimothyGu mentioned this pull request Jul 26, 2017

doc: cross-link util.TextDecoder and intl.md #14486

Closed

2 tasks

bnoordhuis previously approved these changes Jul 28, 2017

View reviewed changes

TimothyGu mentioned this pull request Jul 29, 2017

build: fix build break when omitting icu #14533

Closed

2 tasks

TimothyGu force-pushed the encoding-no-icu branch 4 times, most recently from 626437c to 67493e8 Compare July 30, 2017 11:04

addaleax added the i18n-api Issues and PRs related to the i18n implementation. label Jul 30, 2017

jasnell reviewed Jul 31, 2017

View reviewed changes

jasnell approved these changes Jul 31, 2017

View reviewed changes

refack reviewed Aug 1, 2017

View reviewed changes

TimothyGu force-pushed the encoding-no-icu branch from 67493e8 to fde1f4b Compare August 1, 2017 05:50

refack approved these changes Aug 1, 2017

View reviewed changes

TimothyGu force-pushed the encoding-no-icu branch from d2b9f5d to ad06098 Compare August 2, 2017 00:18

TimothyGu added 2 commits August 2, 2017 09:33

test: fix test-readline-position w/o ICU

939818f

encoding: rudimentary TextDecoder support w/o ICU

e07d268

Also split up the tests.

TimothyGu force-pushed the encoding-no-icu branch from ad06098 to e07d268 Compare August 2, 2017 01:34

inspector: fix console with inspector disabled

27b5568

TimothyGu added the inspector Issues and PRs related to the V8 inspector protocol label Aug 2, 2017

TimothyGu closed this Aug 5, 2017

TimothyGu deleted the encoding-no-icu branch August 5, 2017 08:41

TimothyGu added a commit that referenced this pull request Aug 5, 2017

test: fix test-readline-position w/o ICU

3b0ef0b

PR-URL: #14489 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

TimothyGu added a commit that referenced this pull request Aug 5, 2017

encoding: rudimentary TextDecoder support w/o ICU

365b2e3

Also split up the tests. PR-URL: #14489 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

TimothyGu added a commit that referenced this pull request Aug 5, 2017

inspector: fix console with inspector disabled

17547c4

PR-URL: #14489 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

addaleax pushed a commit that referenced this pull request Aug 7, 2017

test: fix test-readline-position w/o ICU

28b9c7a

PR-URL: #14489 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

addaleax pushed a commit that referenced this pull request Aug 7, 2017

inspector: fix console with inspector disabled

deea68c

PR-URL: #14489 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

addaleax added the backport-requested-v8.x label Aug 10, 2017

fornwall added a commit to termux/termux-packages that referenced this pull request Aug 11, 2017

nodejs-current: Update from 8.2.1 to 8.3.0

552d700

Note that the resulting package does not work yet due to nodejs/node#14489 not being in 8.3.0.

TimothyGu added a commit to TimothyGu/node that referenced this pull request Aug 12, 2017

encoding: rudimentary TextDecoder support w/o ICU

2ab406e

Also split up the tests. PR-URL: nodejs#14489 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

TimothyGu mentioned this pull request Aug 12, 2017

[v8.x backport] encoding: rudimentary TextDecoder support w/o ICU #14786

Closed

4 tasks

addaleax added backported-to-v8.x and removed backport-requested-v8.x labels Aug 12, 2017

addaleax mentioned this pull request Aug 13, 2017

v8.4.0 proposal #14811

Merged

MylesBorins added the lts-watch-v6.x label Aug 16, 2017

MylesBorins added backport-requested-v6.x and removed lts-watch-v6.x labels Sep 19, 2017

Fix building without ICU by TimothyGu · Pull Request #14489 · nodejs/node · GitHub

Uh oh!

Uh oh!

Fix building without ICU - #14489

Fix building without ICU#14489TimothyGu wants to merge 3 commits intonodejs:masternodejs/node:masterfrom TimothyGu:encoding-no-icuTimothyGu/node:encoding-no-icuCopy head branch name to clipboard

Conversation

TimothyGu commented Jul 26, 2017

Checklist

Affected core subsystem(s)

Uh oh!

jasnell commented Jul 27, 2017

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Jul 30, 2017

Uh oh!

TimothyGu commented Jul 30, 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!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

refack 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!

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!

TimothyGu commented Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimothyGu commented Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimothyGu commented Aug 5, 2017

Uh oh!

addaleax commented Aug 10, 2017

Uh oh!

TimothyGu commented Aug 12, 2017

Uh oh!

MylesBorins commented Aug 16, 2017

Uh oh!

MylesBorins commented Sep 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Fix building without ICU#14489
TimothyGu wants to merge 3 commits into
nodejs:masterfrom
TimothyGu:encoding-no-icu

TimothyGu commented Aug 2, 2017 •
edited
Loading

TimothyGu commented Aug 2, 2017 •
edited
Loading