Skip to content

nodejs/node#3111 - v8BreakIterator disablement - #4253

Closed
srl295 wants to merge 1 commit into
nodejs:masterfrom
srl295:nobreak
Closed

nodejs/node#3111 - v8BreakIterator disablement#4253
srl295 wants to merge 1 commit into
nodejs:masterfrom
srl295:nobreak

Conversation

@srl295

@srl295srl295 commented Dec 12, 2015

Copy link
Copy Markdown
Member

Getting this going… need to rebase

@srl295
srl295force-pushed the nobreak branch 2 times, most recently from 81c275d to d8b0b26CompareDecember 12, 2015 01:30
@srl295

srl295 commented Dec 12, 2015

Copy link
Copy Markdown
MemberAuthor

Rebased. Problem, it doesn't actually work. My changes to src/node.js Don't seem to have effect after rebase.

@srl295srl295 self-assigned this Dec 12, 2015
@srl295srl295 added the wip Issues and PRs that are still a work in progress. label Dec 12, 2015
@mscdexmscdex added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 12, 2015
@Fishrock123

Fishrock123 commented Dec 14, 2015

Copy link
Copy Markdown
Contributor

linking: #3111

@srl295srl295 added i18n-api Issues and PRs related to the i18n implementation. and removed wip Issues and PRs that are still a work in progress. labels Apr 9, 2016
@srl295

srl295 commented Apr 9, 2016

Copy link
Copy Markdown
MemberAuthor

OK, it's working again. Can I get a LGTM?

@srl295

srl295 commented Apr 9, 2016

Copy link
Copy Markdown
MemberAuthor

To test:
node -e 'new Intl.v8BreakIterator()'

before: crashes in small-icu (which is our default)
after: no crash

$ ./node -e 'new Intl.v8BreakIterator()'
internal/process.js:72
throw Error('Data not available...'); ^
Error: Data not available...
at Error (native)
at new v8BreakIterator (internal/process.js:72:15)
at [eval]:1:1
at Object.exports.runInThisContext (vm.js:54:17)
at Object.<anonymous> ([eval]-wrapper:6:22)
at Module._compile (module.js:416:34)
at node.js:317:27
at _combinedTickCallback (internal/process/next_tick.js:67:7)
at process._tickCallback (internal/process/next_tick.js:98:9)

@srl295

srl295 commented Apr 9, 2016

Copy link
Copy Markdown
MemberAuthor
  • add test

Question: what's the best way to test this?

bnoordhuis
bnoordhuis reviewed Apr 9, 2016
View reviewed changes
Comment threadlib/internal/process.js Outdated

@bnoordhuisbnoordhuisApr 9, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Style error: space after (. Can you put each check on its own line, followed by the &&? You'll also need to start with:

if(process.config&&process.config.variables&&// ...

@srl295srl295Apr 11, 2016

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Thanks. I'll drop the check for process.config.variables.v8_enable_i18n_support also. For this code's purpose, checking Intl is enough. One fewer dependency on an internal v8 variable is better.

@srl295

srl295 commented Apr 11, 2016

Copy link
Copy Markdown
MemberAuthor

Updated, PTAL

bnoordhuis
bnoordhuis reviewed Apr 11, 2016
View reviewed changes
Comment threadlib/internal/process.js Outdated

@bnoordhuisbnoordhuisApr 11, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You still need a process.config && process.config.variables feature check here and can you make sure make lint passes?

@srl295

srl295 commented Apr 11, 2016

Copy link
Copy Markdown
MemberAuthor
  • linter happy
  • test updated

bnoordhuis
bnoordhuis reviewed Apr 11, 2016
View reviewed changes
Comment threadlib/internal/process.js Outdated

@bnoordhuisbnoordhuisApr 11, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Needs a process.config && process.config.variables feature check.

@estliberitas
estliberitasforce-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fbCompareApril 26, 2016 05:22
@srl295

srl295 commented May 3, 2016

Copy link
Copy Markdown
MemberAuthor

Updated, rebased. Thanks @jasnell

@srl295
intl: Don't crash if v8BreakIterator not available
c99f231
If the undocumented v8BreakIterator does not have data available,
monkeypatch it to throw an error instead of crashing.
Fixesnodejs#3111
@jasnell

jasnell commented May 3, 2016

Copy link
Copy Markdown
Member

LGTM if CI is green :-)

@srl295

srl295 commented May 3, 2016

Copy link
Copy Markdown
MemberAuthor

ci build in progress…

@srl295

srl295 commented May 3, 2016

Copy link
Copy Markdown
MemberAuthor

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

srl295 added a commit that referenced this pull request May 4, 2016
@srl295
intl: Don't crash if v8BreakIterator not available
cd752e8
If the undocumented v8BreakIterator does not have data available,
monkeypatch it to throw an error instead of crashing.
Fixes: #3111
PR-URL: #4253
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@srl295

srl295 commented May 4, 2016

Copy link
Copy Markdown
MemberAuthor

Landed in cd752e8

@srl295srl295 closed this May 4, 2016
@srl295
srl295 deleted the nobreak branch May 4, 2016 22:07
evanlucas pushed a commit that referenced this pull request May 17, 2016
@srl295@evanlucas
intl: Don't crash if v8BreakIterator not available
5966650
If the undocumented v8BreakIterator does not have data available,
monkeypatch it to throw an error instead of crashing.
Fixes: #3111
PR-URL: #4253
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@evanlucasevanlucas mentioned this pull request May 17, 2016
Merged
@MylesBorins

MylesBorins commented Jun 1, 2016

Copy link
Copy Markdown
Contributor

@srl295 this does not land cleanly on v4.x would you be willing to manually backport?

@srl295

srl295 commented Jun 1, 2016•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Yes. Probably an earlier version would. Or maybe depend on back porting the other process change @jasnell ?

@MylesBorins

MylesBorins commented Jun 29, 2016

Copy link
Copy Markdown
Contributor

@srl295 / @jasnell ping

@MylesBorins

MylesBorins commented Aug 30, 2016

Copy link
Copy Markdown
Contributor

@srl295 ping

@srl295srl295 changed the title nodejs/node#3111 - WIP v8BreakIterator disablementnodejs/node#3111 - v8BreakIterator disablementOct 10, 2016
@srl295

srl295 commented Oct 10, 2016

Copy link
Copy Markdown
MemberAuthor

#9008 is the v4.x backport.

MylesBorins pushed a commit that referenced this pull request Oct 10, 2016
@srl295Myles Borins
intl: Don't crash if v8BreakIterator not available
0d53956
If the undocumented v8BreakIterator does not have data available,
monkeypatch it to throw an error instead of crashing.
Fixes: #3111
Ref: #9008
PR-URL: #4253
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Oct 10, 2016
@srl295Myles Borins
intl: Don't crash if v8BreakIterator not available
b06323c
If the undocumented v8BreakIterator does not have data available,
monkeypatch it to throw an error instead of crashing.
Fixes: #3111
Ref: #9008
PR-URL: #4253
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
@srl295@rvagg
intl: Don't crash if v8BreakIterator not available
d5d69fd
If the undocumented v8BreakIterator does not have data available,
monkeypatch it to throw an error instead of crashing.
Fixes: #3111
Ref: #9008
PR-URL: #4253
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
@srl295@MylesBorins
intl: Don't crash if v8BreakIterator not available
2e29b76
If the undocumented v8BreakIterator does not have data available,
monkeypatch it to throw an error instead of crashing.
Fixes: #3111
Ref: #9008
PR-URL: #4253
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorinsMylesBorins mentioned this pull request Oct 26, 2016
Closed
@TimothyGuTimothyGu mentioned this pull request Aug 18, 2017
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@srl295srl295

Labels

i18n-apiIssues and PRs related to the i18n implementation.lib / srcIssues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@srl295@Fishrock123@bnoordhuis@jasnell@MylesBorins@mscdex