FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: Add notes for using @@toStringTag by yosuke-furukawa · Pull Request #2201 · nodejs/node · GitHub

/ node Public

doc: Add notes for using @@toStringTag - #2201

Closed
yosuke-furukawa wants to merge 1 commit into
nodejs:masterfrom
yosuke-furukawa:add_symboltag_note
ConversationCommits (1)ChecksFiles changed
Closed

doc: Add notes for using @@toStringTag#2201
yosuke-furukawa wants to merge 1 commit into
nodejs:masterfrom
yosuke-furukawa:add_symboltag_note

Conversation

yosuke-furukawa commented Jul 18, 2015

Copy link
Copy Markdown
Member

I checked util.is* functions. the following functions may be broken using ES2015 features.

  • util.isDate
  • util.isError
  • util.isRegExp

those functions use Object.prototype.toString.call to check the type.
But @@toStringTag can chage the returned value. @@toStringTag is enabled under --harmony on io.js v2.

For example,

var fakeRegExp = {}; 
fakeRegExp[Symbol.toStringTag] = 'RegExp';
console.log(fakeRegExp.toString()); // [object RegExp]
console.log(util.isRegExp(obj)); // true

var regexp = /a/;
console.log(util.isRegExp(regexp)); // true
RegExp.prototype[Symbol.toStringTag] = 'Array';
console.log(util.isRegExp(regexp)); // false

This PR is to add a notice in these util function.

Fishrock123 commented Jul 18, 2015

Copy link
Copy Markdown
Contributor

@yosuke-furukawa would you mind changing this to inform about this, but also center it around deprecation notices? That would be great, it's something I never got around to doing. :)

Prior discussion: #1301

Fishrock123 added util Issues and PRs related to the built-in util module. doc Issues and PRs related to the documentations. labels Jul 18, 2015

yosuke-furukawa commented Jul 18, 2015

Copy link
Copy Markdown
Member Author

@Fishrock123 +1. We would be better to deprecate util.is* functions in v3. And we will add these notice to deprecation reason.

yosuke-furukawa mentioned this pull request Jul 18, 2015
Closed

Fishrock123 commented Jul 18, 2015

Copy link
Copy Markdown
Contributor

@yosuke-furukawa I wouldn't delay a deprecation notice in the docs for v3 (It was supposed to be in v2 so..)

Besides, we'll probably need to wait a good while before we deprecate this with a run-time warning.

Fishrock123 mentioned this pull request Jul 22, 2015
Closed

jasnell commented Nov 5, 2015

Copy link
Copy Markdown
Member

@Fishrock123 @yosuke-furukawa ... is there reason to keep this one open? Is it still needed? If so, it needs to be updated.

bnoordhuis reviewed Nov 5, 2015
View reviewed changes
Comment thread doc/api/util.markdown Outdated

bnoordhuis Nov 5, 2015

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

"Note:" (capitalize) and please wrap lines at 80 columns. I'd write 'enabled with' instead of 'enabled under'.

bnoordhuis commented Nov 5, 2015

Copy link
Copy Markdown
Member

LGTM when rebased and comments addressed. I think this is a valuable addition to the documentation.

jasnell commented Nov 13, 2015

Copy link
Copy Markdown
Member

@yosuke-furukawa ... could I ask you to please rebase, update and address @bnoordhuis' comments. Thank you!

jasnell added the stalled Issues and PRs that are stalled. label Dec 14, 2015

Trott commented Feb 23, 2016

Copy link
Copy Markdown
Member

This can be closed, I think. I think the problem this warns about has been corrected. The sample code for getting incorrect results seems to now return the correct results. It may have been fixed in 6526ae7.

/cc @cjihrig

If there's something I'm missing, please feel free to re-open.

Trott closed this Feb 23, 2016

cjihrig commented Feb 23, 2016

Copy link
Copy Markdown
Contributor

This is mostly resolved. The isError() function still relies on Object.prototype.toString().

cjihrig mentioned this pull request Feb 24, 2016
Merged
4 tasks
cjihrig added a commit to cjihrig/node that referenced this pull request Feb 25, 2016
doc: note util.isError() @@toStringTag limitations
54cbf28 
util.isError() is the only remaining util.is*() method that
depends on Object.prototype.toString() behavior. This commit
notes the limitations of isError() related to @@toStringTag.

Refs: nodejs#2201
PR-URL: nodejs#5414
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
rvagg pushed a commit that referenced this pull request Feb 27, 2016
doc: note util.isError() @@toStringTag limitations
27a0f87 
util.isError() is the only remaining util.is*() method that
depends on Object.prototype.toString() behavior. This commit
notes the limitations of isError() related to @@toStringTag.

Refs: #2201
PR-URL: #5414
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
rvagg pushed a commit that referenced this pull request Feb 27, 2016
doc: note util.isError() @@toStringTag limitations
e46915f 
util.isError() is the only remaining util.is*() method that
depends on Object.prototype.toString() behavior. This commit
notes the limitations of isError() related to @@toStringTag.

Refs: #2201
PR-URL: #5414
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations. stalled Issues and PRs that are stalled. 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.

6 participants

Back | FazBrowse Home | New Git URL