Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: Add notes for using @@toStringTag #2201

Closed
wants to merge 1 commit into from
Closed

doc: Add notes for using @@toStringTag #2201

wants to merge 1 commit into from

Conversation

yosuke-furukawa
Copy link
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
Copy link
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 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
Copy link
Member Author

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

@Fishrock123
Copy link
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.

@jasnell
Copy link
Member

jasnell commented Nov 5, 2015

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

@@ -194,6 +194,21 @@ Returns `true` if the given "object" is a `RegExp`. `false` otherwise.
util.isRegExp({})
// false

note: This function uses `Object.prototype.toString.call` and check the returned value is `[object RegExp]`. However, `Symbol.toStringTag` can change the returned value directly. `Symbol.toStringTag` is enabled under `--harmony-tostring`.
Copy link
Member

Choose a reason for hiding this comment

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

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

@bnoordhuis
Copy link
Member

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

@jasnell
Copy link
Member

jasnell commented Nov 13, 2015

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

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Dec 14, 2015
@Trott
Copy link
Member

Trott commented Feb 23, 2016

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 Trott closed this Feb 23, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Feb 23, 2016

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

cjihrig added a commit to cjihrig/node that referenced this pull request Feb 25, 2016
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
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
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

6 participants