Skip to content

Conversation

@watilde
Copy link
Member

Like the other lib codes, we should use process.binding('config').hasIntl instead of try-catch to make sure icu is bonded or not.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

url

@nodejs-github-botnodejs-github-bot added the url Issues and PRs related to the legacy built-in url module. label Feb 27, 2017
@JacksonTian
Copy link
Contributor

LGTM

@TimothyGu
Copy link
Member

TimothyGu commented Feb 27, 2017

Slight nit in commit message: IMO "internal modules" sounds better than "lib codes". Otherwise LGTM.

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

@gibfahn
Copy link
Member

Not entirely sure what happened with linuxone in that last CI, rerunning:

CI 2: https://ci.nodejs.org/job/node-test-commit/8137/

EDIT: Passed on rerun

@watilde
Copy link
MemberAuthor

I will update the commit message to replace lib codes with internal modules :)

Like the other internal modules, we should use `process.binding('config').hasIntl` instead of `try-catch` to make sure `icu` is bonded or not.
@watilde
Copy link
MemberAuthor

jasnell pushed a commit that referenced this pull request Mar 4, 2017
Like the other internal modules, we should use `process.binding('config').hasIntl` instead of `try-catch` to make sure `icu` is bonded or not. PR-URL: #11571 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Jackson Tian <[email protected]>
@jasnell
Copy link
Member

Landed in cccc6d8

@jasnelljasnell closed this Mar 4, 2017
@watildewatilde deleted the feature/url-hasIntl branch March 4, 2017 16:15
addaleax pushed a commit that referenced this pull request Mar 5, 2017
Like the other internal modules, we should use `process.binding('config').hasIntl` instead of `try-catch` to make sure `icu` is bonded or not. PR-URL: #11571 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Jackson Tian <[email protected]>
@evanlucasevanlucas mentioned this pull request Mar 8, 2017
MylesBorins pushed a commit that referenced this pull request Apr 17, 2017
Like the other internal modules, we should use `process.binding('config').hasIntl` instead of `try-catch` to make sure `icu` is bonded or not. PR-URL: #11571 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Jackson Tian <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
Like the other internal modules, we should use `process.binding('config').hasIntl` instead of `try-catch` to make sure `icu` is bonded or not. PR-URL: #11571 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Jackson Tian <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Apr 19, 2017
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
Like the other internal modules, we should use `process.binding('config').hasIntl` instead of `try-catch` to make sure `icu` is bonded or not. PR-URL: nodejs/node#11571 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Jackson Tian <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

urlIssues and PRs related to the legacy built-in url module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@watilde@JacksonTian@TimothyGu@gibfahn@jasnell@addaleax@lpinca@cjihrig@MylesBorins@nodejs-github-bot