Skip to content

url: remove usage of require('util') #26808

Closed
toshi1127 wants to merge 1 commit into
nodejs:masterfrom
toshi1127:util_url
Closed

url: remove usage of require('util') #26808
toshi1127 wants to merge 1 commit into
nodejs:masterfrom
toshi1127:util_url

Conversation

@toshi1127

@toshi1127 toshi1127 commented Mar 20, 2019

Copy link
Copy Markdown
Contributor

Remove the usage of public require('util').
Use require('internal/util/inspect').inspect instead of require('util').inspect.
Refs: #26546

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows [commit guidelines]

@joyeecheung joyeecheung mentioned this pull request Mar 20, 2019
Closed
@jasnell

jasnell commented Mar 21, 2019

Copy link
Copy Markdown
Member

@nodejs/tsc ... just a quick note: we need to be clear on how these kinds of changes need to be handled semver-wise. This change, for instance, changes the monkey-patch-ability of util.inspect() in this case. Is that an ok thing to do for a semver-patch or should breaking monkey-patching be considered semver-major always?

@targos

targos commented Mar 21, 2019

Copy link
Copy Markdown
Member

I think we should generally consider this semver-patch. It is not part of the API contract of the URL module that the util.inspect function is used internally in its methods.

@BridgeAR

BridgeAR commented Mar 22, 2019

Copy link
Copy Markdown
Member

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

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 22, 2019
@joyeecheung joyeecheung mentioned this pull request Mar 22, 2019
Closed
@BridgeAR

BridgeAR commented Mar 22, 2019

Copy link
Copy Markdown
Member

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

@ZYSzys

ZYSzys commented Mar 24, 2019

Copy link
Copy Markdown
Member

Landed in 5b59b5f 🎉

@ZYSzys ZYSzys closed this Mar 24, 2019
ZYSzys pushed a commit that referenced this pull request Mar 24, 2019
@toshi1127 @ZYSzys
url: remove usage of require('util')
5b59b5f 
PR-URL: #26808
Refs: #26546
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
@toshi1127 @targos
url: remove usage of require('util')
5b749c7 
PR-URL: nodejs#26808
Refs: nodejs#26546
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
targos pushed a commit that referenced this pull request Mar 27, 2019
@toshi1127 @targos
url: remove usage of require('util')
04c7db3 
PR-URL: #26808
Refs: #26546
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
@targos targos mentioned this pull request Mar 27, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@ZYSzys ZYSzys ZYSzys approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@toshi1127 @jasnell @targos @BridgeAR @ZYSzys