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.

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

Fix building without ICU #14489

Closed
wants to merge 3 commits into from
Closed

Conversation

TimothyGu
Copy link
Member

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

@TimothyGu 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
@jasnell
Copy link
Member

jasnell commented Jul 27, 2017

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

bnoordhuis
bnoordhuis previously approved these changes Jul 28, 2017
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM. Only lightly reviewed, mostly rubber-stamp.

@jasnell That's nodejs/build#419.

@TimothyGu
Copy link
Member Author

@jasnell I assume your comment was a LGTM?

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

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/

@addaleax addaleax added the i18n-api Issues and PRs related to the i18n implementation. label Jul 30, 2017
doc/api/util.md Outdated

* `encoding` {string} Identifies the `encoding` that this `TextDecoder` instance
supports. Defaults to `'utf-8'`.
* `options` {Object}
* `fatal` {boolean} `true` if decoding failures are fatal. Defaults to
`false`.
`false`. This options is only supported when ICU is enabled (see
Copy link
Member

Choose a reason for hiding this comment

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

s/options/option

@@ -14,16 +14,23 @@ const ctrlU = { ctrl: true, name: 'u' };
prompt: ''
});

for (const [cursor, string] of [
const tests = [
Copy link
Member

Choose a reason for hiding this comment

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

This change does not look related?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

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


{
const fn = TextEncoder.prototype[inspect];
fn.call(new TextEncoder(), Infinity, {});
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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


[{}, [], true, 1, '', new TextDecoder()].forEach((i) => {
assert.throws(() => fn.call(i, Infinity, {}),
common.expectsError({
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add the Type of the expected Error.


{
const fn = TextDecoder.prototype[inspect];
fn.call(new TextDecoder(), Infinity, {});
Copy link
Contributor

Choose a reason for hiding this comment

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

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

code: 'ERR_ENCODING_INVALID_ENCODED_DATA',
type: TypeError,
message:
/^The encoded data was not valid for encoding utf-8$/
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -118,6 +118,7 @@ E('ERR_HTTP_HEADERS_SENT',
E('ERR_HTTP_INVALID_STATUS_CODE', 'Invalid status code: %s');
E('ERR_HTTP_TRAILER_INVALID',
'Trailers are invalid with this transfer encoding');
E('ERR_ICU_NOT_SUPPORTED', '%s is not supported on Node.js built without ICU');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@TimothyGu
Copy link
Member Author

TimothyGu commented Aug 2, 2017

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

@TimothyGu
Copy link
Member Author

TimothyGu commented Aug 2, 2017

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/

@TimothyGu TimothyGu added the inspector Issues and PRs related to the V8 inspector protocol label Aug 2, 2017
@TimothyGu
Copy link
Member Author

Landed in 34d1b11...17547c4.

@TimothyGu TimothyGu closed this Aug 5, 2017
@TimothyGu TimothyGu deleted the encoding-no-icu branch August 5, 2017 08:41
TimothyGu added a commit that referenced this pull request Aug 5, 2017
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
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
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
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
PR-URL: #14489
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@addaleax
Copy link
Member

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

fornwall added a commit to termux/termux-packages that referenced this pull request Aug 11, 2017
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
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
Copy link
Member Author

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

addaleax pushed a commit that referenced this pull request Aug 12, 2017
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>
@addaleax addaleax mentioned this pull request Aug 13, 2017
@MylesBorins
Copy link
Contributor

Is this needed on v6.x?

@MylesBorins
Copy link
Contributor

ping. this would need to be manually backported

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
encoding Issues and PRs related to the TextEncoder and TextDecoder APIs. i18n-api Issues and PRs related to the i18n implementation. inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants