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

Intl: Consider deprecating Intl.v8BreakIterator #8865

Closed
jasnell opened this issue Sep 30, 2016 · 21 comments
Closed

Intl: Consider deprecating Intl.v8BreakIterator #8865

jasnell opened this issue Sep 30, 2016 · 21 comments
Labels
discuss Issues opened for discussions and feedbacks. i18n-api Issues and PRs related to the i18n implementation.

Comments

@jasnell
Copy link
Member

jasnell commented Sep 30, 2016

@littledan has put forward a TC-39 proposal to introduce Intl.Segmenter as a standardized replacement for the V8 specific Intl.v8BreakIterator.

Currently in Node.js, Intl.v8BreakIterator only works if the full-icu data is being used. Attempting to create an instance of Intl.v8BreakIterator with the default small-icu with throw. That said, given the desire to replace it with a standardized alternative we should consider emitting a runtime deprecation warning when it is used.

/cc @srl295 @caridy @nodejs/ctc @nodejs/tc39

@jasnell jasnell added discuss Issues opened for discussions and feedbacks. i18n-api Issues and PRs related to the i18n implementation. labels Sep 30, 2016
@srl295
Copy link
Member

srl295 commented Oct 1, 2016

Makes sense… is there a need to deprecate something that's really v8 internal? So I'm +0.0 on this subject to rounding err!

@littledan
Copy link

I hope no one new starts using the deprecated API, but it's currently exposed to some Node users. I'd support a runtime deprecation warning but would be even happier if it were just deleted. I hope to remove it entirely from a future version of V8.

@srl295
Copy link
Member

srl295 commented Oct 1, 2016

Deletion makes sense too. I'm +1 on principle just not sure about policy details.

Enviado desde nuestro iPhone.

El oct. 1, 2016, a las 12:35 PM, littledan notifications@github.com escribió:

I hope no one new starts using the deprecated API, but it's currently exposed to some Node users. I'd support a runtime deprecation warning but would be even happier if it were just deleted. I hope to remove it entirely from a future version of V8.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@jasnell
Copy link
Member Author

jasnell commented Oct 2, 2016

We have to deprecate first in one major before we can remove in the next.

On Saturday, October 1, 2016, Steven R. Loomis notifications@github.com
wrote:

Deletion makes sense too. I'm +1 on principle just not sure about policy
details.

Enviado desde nuestro iPhone.

El oct. 1, 2016, a las 12:35 PM, littledan <notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');> escribió:

I hope no one new starts using the deprecated API, but it's currently
exposed to some Node users. I'd support a runtime deprecation warning but
would be even happier if it were just deleted. I hope to remove it entirely
from a future version of V8.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#8865 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eeHBPMPPYSlP9a66MhHwOGTR76LPks5qvs4KgaJpZM4KLXGH
.

@srl295
Copy link
Member

srl295 commented Oct 2, 2016 via email

@caridy
Copy link

caridy commented Oct 3, 2016

@srl295 I thought we had discussed this in the past, and agreed on throwing an error, at least that was my understanding. Anyhow, let's try to take action asap, to prevent a bigger problem later on.

@Fishrock123
Copy link
Contributor

If there is a chance this will be removed before the next major we should deprecate in v7.

@jasnell
Copy link
Member Author

jasnell commented Oct 3, 2016

I'm thinking we probably should. I'm not able to add the ctc-agenda label
at the moment. We would need ctc ok to land it in v7.

On Monday, October 3, 2016, Jeremiah Senkpiel notifications@github.com
wrote:

If there is a chance this will be removed before the next major we should
deprecate in v7.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#8865 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2ef5BMHzosnWYo6fyrDTuOeNRWNSuks5qwQgwgaJpZM4KLXGH
.

@targos
Copy link
Member

targos commented Oct 3, 2016

Here is a PR adding the deprecation warning: #8908

@rvagg
Copy link
Member

rvagg commented Oct 5, 2016

One real problem here is that if v8BreakIterator gets removed in a near-future version of V8 then it will increase the difficulty in upgrading V8 mid-major, we'll either have to polyfill/backfill in a patched version of V8 when we upgrade to avoid breakage or break for users who may be using it (do we really think folks will be using it?).

The problem here is the speculative nature of the problem here.

Intl.Segmenter is before TC39, but that could take many months, no?

and

@littledan:

I hope to remove it entirely from a future version of V8.

What timeframe might we be talking about? Would this depend on TC39? What if Intl.Segmenter doesn't get accepted by TC39? Will Intl.v8BreakIterator be removed regardless?

@rvagg
Copy link
Member

rvagg commented Oct 5, 2016

Some discussion at CTC meeting, lots of uncertainty still, @littledan is on vacation for a few weeks apparently. This should come back on the agenda.

@rvagg
Copy link
Member

rvagg commented Oct 5, 2016

@s3ththompson: "won't be deprecating it until Intl.Segmenter makes it in, but would be happy for Node to remove it to be forward-looking, particularly since it only impacts a subset of users."

@jasnell
Copy link
Member Author

jasnell commented Oct 5, 2016

At the very least, adding the deprecation warning is the right thing to do
now just in case

On Wednesday, October 5, 2016, Rod Vagg notifications@github.com wrote:

@s3ththompson https://github.com/s3ththompson: "won't be deprecating it
until Intl.Segmenter makes it in, but would be happy for Node to remove it
to be forward-looking, particularly since it only impacts a subset of
users."


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#8865 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eQUvMJl8eBJ-mSqnpCro7GWzjbpvks5qxA0_gaJpZM4KLXGH
.

@jasnell jasnell closed this as completed in 105e628 Oct 7, 2016
jasnell pushed a commit that referenced this issue Oct 7, 2016
Fixes: #8865
PR-URL: #8908
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@littledan
Copy link

To give a little more context on top of what @s3ththompson, Intl.Segmenter will take at least six more months, and could be even longer (a year or two), but I'm optimistic that it'll eventually make it through standardization.

@Trott Trott removed the ctc-agenda label Oct 20, 2016
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Sep 8, 2017
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
addaleax pushed a commit that referenced this issue Sep 10, 2017
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 <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit to addaleax/node that referenced this issue Sep 13, 2017
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 <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@AaronPresley
Copy link

AaronPresley commented Apr 8, 2019

Hi all,

I'm looking to integrate segmentation in some node (v10) code.

From what I understand after a twitter chat with @srl295, Intl.v8BreakIterator has been completely removed; however its replacement of Intl.Segmentation is still in Stage 3 and not yet approved.

How would you recommend I use these APIs in node? Am I just stuck with an older version that supports Intl.v8BreakIterator? Would appreciate some guidance.

Secondly, would also love to understand why this feature was completely removed before a suitable replacement was approved? Or am I misunderstanding something?

Thanks for your time.

@devsnek
Copy link
Member

devsnek commented Apr 8, 2019

if you only need en-US, this should work. https://gist.github.com/inexorabletash/8c4d869a584bcaa18514729332300356

@AaronPresley
Copy link

@devsnek Unfortunately en-US is the least of my worries. Mostly working with CJK languages.

@srl295
Copy link
Member

srl295 commented Apr 8, 2019

why this feature was completely removed before a suitable replacement was approved?

Because it wasn't actually a Node.js feature. It was an unsupported, lightly documented undocumented, non standard, internal function in v8. It tended to cause a process crash (#3111) rather than a graceful response to data loading or parameter errors.

A couple of other interim options which link against another copy of ICU4C might include: https://www.npmjs.com/package/node-icu-tokenizer or https://www.npmjs.com/package/icu-wordsplit — maybe these could be made to work with the new API.

@AaronPresley
Copy link

AaronPresley commented Apr 8, 2019

Because it wasn't actually a Node.js feature.

Ah, that makes sense then. Thanks for the explanation. That would also explain why it's been so impossible to find documentation for that function :)

@littledan
Copy link

It used to be documented in a Google Code repository, but that was removed when Google Code was shut down, and I'm not sure where to find it in the Internet Archive :(

@srl295
Copy link
Member

srl295 commented Apr 8, 2019

@littledan My mistake! I updated my commend above with a link to the docs ☝️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. i18n-api Issues and PRs related to the i18n implementation.
Projects
None yet
Development

No branches or pull requests

10 participants