Skip to content

util: eliminate unecessary internal/util export#11455

Closed
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:remove-objecttostring
Closed

util: eliminate unecessary internal/util export#11455
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:remove-objecttostring

Conversation

@jasnell

@jasnell jasnell commented Feb 18, 2017

Copy link
Copy Markdown
Member

The objectToString method was only being used in one place
within internalUtil. It didn't make sense to have it as
a separate exported mnethod.

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

internal util

@jasnell
util: eliminate unecessary internal/util export
889e791 
The objectToString method was only being used in one place
within internalUtil. It didn't make sense to have it as
a separate exported mnethod.
@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Feb 18, 2017
@jasnell

jasnell commented Feb 18, 2017

Copy link
Copy Markdown
Member Author

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

@targos

targos commented Feb 18, 2017

Copy link
Copy Markdown
Member

#11128 uses objectToString

@jasnell

jasnell commented Feb 18, 2017

Copy link
Copy Markdown
Member Author

hmm... could it not simply define it's own objectToString or use Object.prototype.toString() directly?

@jasnell

jasnell commented Feb 18, 2017

Copy link
Copy Markdown
Member Author

(thanks for pointing that out, btw :-) ...)

@targos

targos commented Feb 18, 2017

Copy link
Copy Markdown
Member

could it not simply define it's own objectToString or use Object.prototype.toString() directly

absolutely. /cc @joyeecheung

targos
targos approved these changes Feb 18, 2017
View reviewed changes

@targos targos left a comment

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.

LGTM this function doesn't add much value

@joyeecheung

joyeecheung commented Feb 18, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

The reason to use objectToString is in #10282 (comment) , the util can do the caching part at startup(or even get the untampered with version through bindings)

@jasnell

jasnell commented Feb 19, 2017

Copy link
Copy Markdown
Member Author

hmm... I'll hold on this then. I've been thinking through a solution on trapping the instances of classes we depend on. I'll see if I can get that PR in later this week.

@jasnell

jasnell commented Feb 23, 2017

Copy link
Copy Markdown
Member Author

I'm going to go ahead and close this without further action. There are other places where this function is looking to be used.

@jasnell jasnell closed this Feb 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@targos targos targos approved these changes

Assignees

No one assigned

Labels

util Issues and PRs related to the built-in util module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jasnell @targos @joyeecheung @nodejs-github-bot