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

intl: unexpose Intl.v8BreakIterator #15238

Closed
wants to merge 1 commit into from
Closed

intl: unexpose Intl.v8BreakIterator #15238

wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Member

It was never an official Ecma-402 API, it is about to be superseded
by Intl.Segmenter and it's prone to crash under some circumstances.

Searches don't turn up any usage in the wild and the recommendation
from the V8 team is to remove it so now seems like a good a time as
any to do just that.

Fixes: #8865
Fixes: #14909
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/620755

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. process Issues and PRs related to the process subsystem. labels Sep 7, 2017
@targos targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 7, 2017
@TimothyGu
Copy link
Member

Does this actually fix #14909, when the property delete is in lib/internal/process.js, so the main context only?

TimothyGu
TimothyGu reviewed Sep 7, 2017
View reviewed changes
lib/internal/process.js Outdated
'DEP0017');
Object.defineProperty(Intl, 'v8BreakIterator', des);
}
if (typeof Intl !== 'undefined' && Intl.hasOwnProperty('v8BreakIterator'))
Copy link
Member

Choose a reason for hiding this comment

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

Is the hasOwnProperty check necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. I kept it to keep the logic more or less the same but I'll remove it.

@bnoordhuis
Copy link
Member Author

Does this actually fix #14909

Sorry, should have been a Refs:

@mscdex mscdex added the i18n-api Issues and PRs related to the i18n implementation. label Sep 7, 2017
@bnoordhuis
Copy link
Member Author

@TimothyGu That did give me an idea on how to approach it more holistically from C++. PTAL!

jasnell
jasnell approved these changes Sep 7, 2017
View reviewed changes
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.

Lgtm but the text in the deprecations.md section could be updated to not say "will be removed" to "has been removed"... Perhaps with a reference to Intl.Segmenter

TimothyGu
TimothyGu reviewed Sep 7, 2017
View reviewed changes
src/node_internals.h Outdated
@@ -121,6 +121,11 @@ inline v8::Local<TypeName> PersistentToLocal(
v8::Isolate* isolate,
const v8::Persistent<TypeName>& persistent);

v8::Local<v8::Context> NewContext(
Copy link
Member

Choose a reason for hiding this comment

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

Some documentation would be helpful so that reader of the code would not need to look it up in node.cc.

@bnoordhuis
intl: unexpose Intl.v8BreakIterator
c770936 
It was never an official Ecma-402 API, it is about to be superseded
by `Intl.Segmenter` and it's prone to crash under some circumstances.

Searches don't turn up any usage in the wild and the recommendation
from the V8 team is to remove it.  Now seems like a good a time as
any to do that.

Fixes: nodejs#8865
Fixes: nodejs#14909
Refs: https://github.com/tc39/proposal-intl-segmenter
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/620755
@bnoordhuis
Copy link
Member Author

Updated with suggestions. I also fixed a logic error in the test that was carried over from the old test because I wanted to keep the diff small...

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

@addaleax
Copy link
Member

Landed in 668ad44

@addaleax addaleax closed this Sep 10, 2017
addaleax pushed a commit that referenced this pull request Sep 10, 2017
@bnoordhuis @addaleax
intl: unexpose Intl.v8BreakIterator
668ad44 
It was never an official Ecma-402 API, it is about to be superseded
by `Intl.Segmenter` and it's prone to crash under some circumstances.

Searches don't turn up any usage in the wild and the recommendation
from the V8 team is to remove it.  Now seems like a good a time as
any to do that.

Fixes: #8865
Fixes: #14909
Refs: https://github.com/tc39/proposal-intl-segmenter
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/620755
PR-URL: #15238
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@TimothyGu
Copy link
Member

TimothyGu commented Sep 10, 2017 •
edited
Loading

Just a question: has anyone tested that this works on no-Intl builds?

Edit:

addaleax | TimothyGu: so it turns out --without-intl makes nearly all tests fail on master anyway
         | because for some reason that affects the fs-vs-internal/fs circular dependency that we
         | have rn

@bnoordhuis bnoordhuis mentioned this pull request Sep 11, 2017
Closed
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
@bnoordhuis @addaleax
intl: unexpose Intl.v8BreakIterator
53ae3fe 
It was never an official Ecma-402 API, it is about to be superseded
by `Intl.Segmenter` and it's prone to crash under some circumstances.

Searches don't turn up any usage in the wild and the recommendation
from the V8 team is to remove it.  Now seems like a good a time as
any to do that.

Fixes: nodejs#8865
Fixes: nodejs#14909
Refs: https://github.com/tc39/proposal-intl-segmenter
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/620755
PR-URL: nodejs#15238
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
jasnell added a commit that referenced this pull request Oct 17, 2017
@jasnell
2017-10-31, Version 9.0.0 (Current)
6891ac2 
* **async_hooks**
  * Older experimental `async_hooks` APIs have been removed
    [[`d731369b1d`](d731369b1d)]
    **(SEMVER-MAJOR)** [#14414](#14414)

* **Errors**
  * Multiple built in modules have been migrated to use static error codes

* **Domains**
  * The long deprecated `.dispose()` method has been removed
    [[`602fd36d95`](602fd36d95)]
    **(SEMVER-MAJOR)** [#15412](#15412)

* **File system**
  * `fs.ReadStream` and `fs.WriteStream` now make use of `.destroy()`
    [[`e5c290bed9`](e5c290bed9)]
    **(SEMVER-MAJOR)** [#15407](#15407)
  * `fs` callbacks are now invoked with an undefined `this` context
    [[`2249234fee`](2249234fee)]
    **(SEMVER-MAJOR)** [#14645](#14645)

* **HTTP**
  * Socket timeout is set when the socket connects
    [[`10be20a0e8`](10be20a0e8)]
    **(SEMVER-MAJOR)** [#8895](#8895)
  * A bug causing the request `error` event to fire twice has been fixed
    [[`620ba41694`](620ba41694)]
    **(SEMVER-MAJOR)** [#14659](#14659)
  * The `pipe` method on `OutgoingMessage` has been disabled
    [[`156549d8ff`](156549d8ff)]
    **(SEMVER-MAJOR)** [#14358](#14358)

* **HTTP/2**
  * The `--expose-http2` command-line argument is no longer required
    [[`f55ee6e24a`](f55ee6e24a)]
    **(SEMVER-MAJOR)** [#15535](#15535)

* **Internationalization**
  * The `Intl.v8BreakIterator` class has been removed
    [[`668ad44922`](668ad44922)]
    **(SEMVER-MAJOR)** [#15238](#15238)

* **OS**
  * `os.EOL` is now read-only
    [[`f6caeb9526`](f6caeb9526)]
    **(SEMVER-MAJOR)** [#14622](#14622)

* **Process**
  * It is now possible to pass additional flags to `dlopen`
    [[`5f22375922`](5f22375922)]
    **(SEMVER-MAJOR)** [#12794](#12794)

* **Timers**
  * Using a timeout duration larger than 32-bits will now emit a warning
    [[`ce3586da31`](ce3586da31)]
    **(SEMVER-MAJOR)** [#15627](#15627)

* **TLS**
  * `parseCertString` has been deprecated
    [[`468110b327`](468110b327)]
    **(SEMVER-MAJOR)** [#14249](#14249)
  * Type-checking for `key`, `cert`, and `ca` options has been added
    [[`a7dccd040d`](a7dccd040d)]
    **(SEMVER-MAJOR)** [#14807](#14807)
jasnell added a commit that referenced this pull request Oct 29, 2017
@jasnell
2017-10-31, Version 9.0.0 (Current)
7165af5 
* **async_hooks**
  * Older experimental `async_hooks` APIs have been removed
    [[`d731369b1d`](d731369b1d)]
    **(SEMVER-MAJOR)** [#14414](#14414)

* **Errors**
  * Multiple built in modules have been migrated to use static error codes

* **Domains**
  * The long deprecated `.dispose()` method has been removed
    [[`602fd36d95`](602fd36d95)]
    **(SEMVER-MAJOR)** [#15412](#15412)

* **File system**
  * `fs.ReadStream` and `fs.WriteStream` now make use of `.destroy()`
    [[`e5c290bed9`](e5c290bed9)]
    **(SEMVER-MAJOR)** [#15407](#15407)
  * `fs` callbacks are now invoked with an undefined `this` context
    [[`2249234fee`](2249234fee)]
    **(SEMVER-MAJOR)** [#14645](#14645)

* **HTTP**
  * Socket timeout is set when the socket connects
    [[`10be20a0e8`](10be20a0e8)]
    **(SEMVER-MAJOR)** [#8895](#8895)
  * A bug causing the request `error` event to fire twice has been fixed
    [[`620ba41694`](620ba41694)]
    **(SEMVER-MAJOR)** [#14659](#14659)
  * The `pipe` method on `OutgoingMessage` has been disabled
    [[`156549d8ff`](156549d8ff)]
    **(SEMVER-MAJOR)** [#14358](#14358)

* **HTTP/2**
  * The `--expose-http2` command-line argument is no longer required
    [[`f55ee6e24a`](f55ee6e24a)]
    **(SEMVER-MAJOR)** [#15535](#15535)

* **Internationalization**
  * The `Intl.v8BreakIterator` class has been removed
    [[`668ad44922`](668ad44922)]
    **(SEMVER-MAJOR)** [#15238](#15238)

* **OS**
  * `os.EOL` is now read-only
    [[`f6caeb9526`](f6caeb9526)]
    **(SEMVER-MAJOR)** [#14622](#14622)

* **Process**
  * It is now possible to pass additional flags to `dlopen`
    [[`5f22375922`](5f22375922)]
    **(SEMVER-MAJOR)** [#12794](#12794)

* **Timers**
  * Using a timeout duration larger than 32-bits will now emit a warning
    [[`ce3586da31`](ce3586da31)]
    **(SEMVER-MAJOR)** [#15627](#15627)

* **TLS**
  * `parseCertString` has been deprecated
    [[`468110b327`](468110b327)]
    **(SEMVER-MAJOR)** [#14249](#14249)
  * Type-checking for `key`, `cert`, and `ca` options has been added
    [[`a7dccd040d`](a7dccd040d)]
    **(SEMVER-MAJOR)** [#14807](#14807)
jasnell added a commit that referenced this pull request Oct 29, 2017
@jasnell
2017-10-31, Version 9.0.0 (Current)
a618552 
* **async_hooks**
  * Older experimental `async_hooks` APIs have been removed
    [[`d731369b1d`](d731369b1d)]
    **(SEMVER-MAJOR)** [#14414](#14414)

* **Errors**
  * Multiple built in modules have been migrated to use static error codes

* **Domains**
  * The long deprecated `.dispose()` method has been removed
    [[`602fd36d95`](602fd36d95)]
    **(SEMVER-MAJOR)** [#15412](#15412)

* **File system**
  * `fs.ReadStream` and `fs.WriteStream` now make use of `.destroy()`
    [[`e5c290bed9`](e5c290bed9)]
    **(SEMVER-MAJOR)** [#15407](#15407)
  * `fs` callbacks are now invoked with an undefined `this` context
    [[`2249234fee`](2249234fee)]
    **(SEMVER-MAJOR)** [#14645](#14645)

* **HTTP**
  * Socket timeout is set when the socket connects
    [[`10be20a0e8`](10be20a0e8)]
    **(SEMVER-MAJOR)** [#8895](#8895)
  * A bug causing the request `error` event to fire twice has been fixed
    [[`620ba41694`](620ba41694)]
    **(SEMVER-MAJOR)** [#14659](#14659)
  * The `pipe` method on `OutgoingMessage` has been disabled
    [[`156549d8ff`](156549d8ff)]
    **(SEMVER-MAJOR)** [#14358](#14358)

* **HTTP/2**
  * The `--expose-http2` command-line argument is no longer required
    [[`f55ee6e24a`](f55ee6e24a)]
    **(SEMVER-MAJOR)** [#15535](#15535)

* **Internationalization**
  * The `Intl.v8BreakIterator` class has been removed
    [[`668ad44922`](668ad44922)]
    **(SEMVER-MAJOR)** [#15238](#15238)

* **OS**
  * `os.EOL` is now read-only
    [[`f6caeb9526`](f6caeb9526)]
    **(SEMVER-MAJOR)** [#14622](#14622)

* **Process**
  * It is now possible to pass additional flags to `dlopen`
    [[`5f22375922`](5f22375922)]
    **(SEMVER-MAJOR)** [#12794](#12794)

* **Timers**
  * Using a timeout duration larger than 32-bits will now emit a warning
    [[`ce3586da31`](ce3586da31)]
    **(SEMVER-MAJOR)** [#15627](#15627)

* **TLS**
  * `parseCertString` has been deprecated
    [[`468110b327`](468110b327)]
    **(SEMVER-MAJOR)** [#14249](#14249)
  * Type-checking for `key`, `cert`, and `ca` options has been added
    [[`a7dccd040d`](a7dccd040d)]
    **(SEMVER-MAJOR)** [#14807](#14807)
jasnell added a commit that referenced this pull request Oct 29, 2017
@jasnell
2017-10-31, Version 9.0.0 (Current)
8d28704 
* **async_hooks**
  * Older experimental `async_hooks` APIs have been removed
    [[`d731369b1d`](d731369b1d)]
    **(SEMVER-MAJOR)** [#14414](#14414)

* **Errors**
  * Multiple built in modules have been migrated to use static error codes

* **Domains**
  * The long deprecated `.dispose()` method has been removed
    [[`602fd36d95`](602fd36d95)]
    **(SEMVER-MAJOR)** [#15412](#15412)

* **File system**
  * `fs.ReadStream` and `fs.WriteStream` now make use of `.destroy()`
    [[`e5c290bed9`](e5c290bed9)]
    **(SEMVER-MAJOR)** [#15407](#15407)
  * `fs` callbacks are now invoked with an undefined `this` context
    [[`2249234fee`](2249234fee)]
    **(SEMVER-MAJOR)** [#14645](#14645)

* **HTTP**
  * Socket timeout is set when the socket connects
    [[`10be20a0e8`](10be20a0e8)]
    **(SEMVER-MAJOR)** [#8895](#8895)
  * A bug causing the request `error` event to fire twice has been fixed
    [[`620ba41694`](620ba41694)]
    **(SEMVER-MAJOR)** [#14659](#14659)
  * The `pipe` method on `OutgoingMessage` has been disabled
    [[`156549d8ff`](156549d8ff)]
    **(SEMVER-MAJOR)** [#14358](#14358)

* **HTTP/2**
  * The `--expose-http2` command-line argument is no longer required
    [[`f55ee6e24a`](f55ee6e24a)]
    **(SEMVER-MAJOR)** [#15535](#15535)

* **Internationalization**
  * The `Intl.v8BreakIterator` class has been removed
    [[`668ad44922`](668ad44922)]
    **(SEMVER-MAJOR)** [#15238](#15238)

* **OS**
  * `os.EOL` is now read-only
    [[`f6caeb9526`](f6caeb9526)]
    **(SEMVER-MAJOR)** [#14622](#14622)

* **Process**
  * It is now possible to pass additional flags to `dlopen`
    [[`5f22375922`](5f22375922)]
    **(SEMVER-MAJOR)** [#12794](#12794)

* **Timers**
  * Using a timeout duration larger than 32-bits will now emit a warning
    [[`ce3586da31`](ce3586da31)]
    **(SEMVER-MAJOR)** [#15627](#15627)

* **TLS**
  * `parseCertString` has been deprecated
    [[`468110b327`](468110b327)]
    **(SEMVER-MAJOR)** [#14249](#14249)
  * Type-checking for `key`, `cert`, and `ca` options has been added
    [[`a7dccd040d`](a7dccd040d)]
    **(SEMVER-MAJOR)** [#14807](#14807)
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
@gibfahn gibfahn mentioned this pull request Dec 19, 2017
Closed
4 tasks
@srl295 srl295 mentioned this pull request Dec 4, 2018
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimothyGu TimothyGu TimothyGu left review comments

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. i18n-api Issues and PRs related to the i18n implementation. process Issues and PRs related to the process subsystem. semver-major PRs 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.
7 participants
@bnoordhuis @TimothyGu @addaleax @jasnell @mscdex @targos @nodejs-github-bot