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

doc: note util.isError() @@toStringTag limitations by cjihrig · Pull Request #5414 · nodejs/node · GitHub

/ node Public

doc: note util.isError() @@toStringTag limitations - #5414

Merged
cjihrig merged 1 commit into
nodejs:masterfrom
cjihrig:iserror
Feb 25, 2016
ConversationCommits (1)ChecksFiles changed
Merged

doc: note util.isError() @@toStringTag limitations#5414
cjihrig merged 1 commit into
nodejs:masterfrom
cjihrig:iserror

Conversation

cjihrig commented Feb 24, 2016

Copy link
Copy Markdown
Contributor

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

doc

Description of change

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

mscdex added util Issues and PRs related to the built-in util module. doc Issues and PRs related to the documentations. labels Feb 24, 2016
thefourtheye reviewed Feb 24, 2016
View reviewed changes
Comment thread doc/api/util.markdown Outdated

thefourtheye Feb 24, 2016

Copy link
Copy Markdown
Contributor

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

Should we even document something based on a feature which is behind a flag?

cjihrig Feb 24, 2016

Copy link
Copy Markdown
Contributor Author

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

Normally, I'd say no. But this is already possible with things like babel-node, and is only a matter of time until it's possible without a flag (http://v8project.blogspot.com/2016/01/v8-release-49.html).

thefourtheye Feb 24, 2016

Copy link
Copy Markdown
Contributor

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

Aren't we deprecating all these functions with #1301 (hopefully by v6.0)?

Trott Feb 24, 2016

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

@thefourtheye asks:

Aren't we deprecating all these functions with #1301 (hopefully by v6.0)?

Deprecated functions still need to be accurately documented. In this particular case, if the additional material provided here makes it clear that the function is not bullet-proof, then we're doing people a favor. (I'm not actually sure how likely it is that someone will run across the problem described here, but it's an awfully big ecosystem out there...)

cjihrig Feb 24, 2016

Copy link
Copy Markdown
Contributor Author

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

Yes. They are already deprecated in the docs. I think the warning is still worthwhile until they are no longer exposed from core.

thefourtheye Feb 24, 2016

Copy link
Copy Markdown
Contributor

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

Okay, in that case, this particular thing is applicable to few other functions in util. Can we put this as a separate section and link it in all the relevant function descriptions?

cjihrig Feb 24, 2016

Copy link
Copy Markdown
Contributor Author

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

It should only still be applicable to isError(). The others were moved to checks at the C++ level.

thefourtheye Feb 24, 2016

Copy link
Copy Markdown
Contributor

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

Oops, sorry. I should have checked that first.

thefourtheye commented Feb 24, 2016

Copy link
Copy Markdown
Contributor

LGTM

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>
cjihrig merged commit 54cbf28 into nodejs:master Feb 25, 2016
cjihrig deleted the iserror branch February 25, 2016 14:26

Trott commented Feb 25, 2016

Copy link
Copy Markdown
Member

Landed in 54cbf28

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>
Fishrock123 mentioned this pull request Mar 1, 2016
Merged
5 tasks
MylesBorins mentioned this pull request Mar 10, 2016
Closed

MylesBorins commented Mar 10, 2016

Copy link
Copy Markdown
Contributor

This is applicable to LTS correct?

jasnell commented Mar 11, 2016

Copy link
Copy Markdown
Member

Don't believe this one is relevant for LTS

cjihrig commented Mar 11, 2016

Copy link
Copy Markdown
Contributor Author

Not applicable to LTS

cjihrig added a commit to cjihrig/node that referenced this pull request Apr 30, 2016
doc: remove obsolete comment in isError() example
d4f147e 
One of the util.isError() examples states that a harmony flag
is required. As of v6.0.0, this is no longer true. This commit
removes the out of date reference.

Refs: nodejs#5414
cjihrig mentioned this pull request Apr 30, 2016
Closed
2 tasks
jasnell pushed a commit that referenced this pull request May 1, 2016
doc: remove obsolete comment in isError() example
d09c972 
One of the util.isError() examples states that a harmony flag
is required. As of v6.0.0, this is no longer true. This commit
removes the out of date reference.

Refs: #5414
PR-URL: #6486
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Fishrock123 pushed a commit that referenced this pull request May 4, 2016
doc: remove obsolete comment in isError() example
3e028a1 
One of the util.isError() examples states that a harmony flag
is required. As of v6.0.0, this is no longer true. This commit
removes the out of date reference.

Refs: #5414
PR-URL: #6486
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request May 4, 2016
doc: remove obsolete comment in isError() example
5617bfa 
One of the util.isError() examples states that a harmony flag
is required. As of v6.0.0, this is no longer true. This commit
removes the out of date reference.

Refs: nodejs#5414
PR-URL: nodejs#6486
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@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. 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