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

module: always decorate thrown errors by mscdex · Pull Request #4287 · nodejs/node · GitHub

/ node Public

module: always decorate thrown errors - #4287

Closed
mscdex wants to merge 1 commit into
nodejs:masterfrom
mscdex:module-decorate-errors
ConversationCommits (1)ChecksFiles changed
Closed

module: always decorate thrown errors#4287
mscdex wants to merge 1 commit into
nodejs:masterfrom
mscdex:module-decorate-errors

Conversation

mscdex commented Dec 15, 2015

Copy link
Copy Markdown
Contributor

This provides more information when encountering a syntax or similar error when executing a file with require().

mscdex added the module Issues and PRs related to the module subsystem. label Dec 15, 2015
mscdex mentioned this pull request Dec 15, 2015
Closed

mscdex commented Dec 15, 2015

Copy link
Copy Markdown
Contributor Author

CI is all green except flaky tests and CI issues: https://ci.nodejs.org/job/node-test-pull-request/1001/

bnoordhuis commented Dec 15, 2015

Copy link
Copy Markdown
Member

LGTM

cjihrig commented Dec 15, 2015

Copy link
Copy Markdown
Contributor

Using the normal workflow, this will cause two arrow messages to be printed. For example:

$ node foo.js 
/Users/cjihrig/iojs/node/foo.js:1
 ^
 ^

/Users/cjihrig/iojs/node/foo.js:1
 ^
 ^
SyntaxError: Unexpected token ^
...

Where foo.js just contains a ^.

cjihrig commented Dec 15, 2015

Copy link
Copy Markdown
Contributor

From a quick test, it looks like removing the arrow information from ReportException() in node.cc fixes the problem.

mscdex commented Dec 15, 2015

Copy link
Copy Markdown
Contributor Author

Hrmm... what about adding/checking a "decorated" flag as another hidden value on the error object?

mscdex force-pushed the module-decorate-errors branch 2 times, most recently from 57a9169 to 490c4b8 Compare December 15, 2015 20:39

mscdex commented Dec 15, 2015

Copy link
Copy Markdown
Contributor Author

@cjihrig Alright, I added a 'decorated' flag and a new test for uncaught errors.

CI is all green except flaky tests/CI issues: https://ci.nodejs.org/job/node-test-pull-request/1006/

mscdex force-pushed the module-decorate-errors branch from 490c4b8 to fbcb95a Compare December 15, 2015 21:10
cjihrig reviewed Dec 16, 2015
View reviewed changes
Comment thread test/parallel/test-util-decorate-error-stack.js Outdated

cjihrig Dec 16, 2015

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 this use result?

mscdex Dec 16, 2015

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

Fixed.

mscdex force-pushed the module-decorate-errors branch 3 times, most recently from 9dc88d1 to ed203d0 Compare December 16, 2015 03:56

mscdex commented Dec 16, 2015

Copy link
Copy Markdown
Contributor Author

Incorporated suggestions and CI is green except flaky tests: https://ci.nodejs.org/job/node-test-pull-request/1010/

cjihrig commented Dec 16, 2015

Copy link
Copy Markdown
Contributor

LGTM

bnoordhuis reviewed Dec 16, 2015
View reviewed changes
Comment thread lib/internal/util.js Outdated

bnoordhuis Dec 16, 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

I'd probably give this a slightly more unique name, e.g. 'node:decorated'. (Ditto for 'arrowMessage' although this PR didn't introduce that.)

jasnell commented Dec 16, 2015

Copy link
Copy Markdown
Member

semver-minor?

mscdex force-pushed the module-decorate-errors branch from ed203d0 to ac1633b Compare December 17, 2015 22:49

mscdex commented Dec 17, 2015

Copy link
Copy Markdown
Contributor Author

@bnoordhuis I've made your suggested changes. LGTY now?

bnoordhuis reviewed Dec 17, 2015
View reviewed changes
Comment thread src/node.cc Outdated

bnoordhuis Dec 17, 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

Superfluous parentheses and you can shorten it to return !decorated.IsEmpty() && decorated->IsTrue(), which should also be infinitesimally faster.

bnoordhuis commented Dec 17, 2015

Copy link
Copy Markdown
Member

LGTM with a nit and a suggestion.

module: always decorate thrown errors
e7ac2fc 
This provides more information when encountering a syntax or similar
error when executing a file with require().
mscdex force-pushed the module-decorate-errors branch from ac1633b to e7ac2fc Compare December 17, 2015 23:54

mscdex commented Dec 18, 2015

Copy link
Copy Markdown
Contributor Author

Updated commit. Last CI run: https://ci.nodejs.org/job/node-test-pull-request/1022/

mscdex commented Dec 18, 2015

Copy link
Copy Markdown
Contributor Author

@jasnell I'm not sure how this fits in (if at all) with whatever was agreed upon on with regard to changing errors and semver-ness, but my guess would be semver-major since it's changing the formatting of the stack trace?

mscdex added a commit that referenced this pull request Dec 19, 2015
module: always decorate thrown errors
18490d3 
This provides more information when encountering a syntax or similar
error when executing a file with require().

Fixes: #4286
PR-URL: #4287
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

mscdex commented Dec 19, 2015

Copy link
Copy Markdown
Contributor Author

Landed in 18490d3.

mscdex closed this Dec 19, 2015
mscdex deleted the module-decorate-errors branch December 19, 2015 19:20
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Dec 22, 2015
module: always decorate thrown errors
72b6b4f 
This provides more information when encountering a syntax or similar
error when executing a file with require().

Fixes: nodejs#4286
PR-URL: nodejs#4287
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Dec 22, 2015
2015-12-22, Version 5.3.1 (Stable)
b7fd395 
Notable changes:

* general: Several minor performance improvements:
  - lib: Use arrow functions instead of bind where possible (Minwoo
Jung) nodejs#3622
  - node: Improved accessor perf of process.env (Trevor Norris)
nodejs#3780
  - **node**: Improved performance of hrtime() (Trevor Norris)
nodejs#3780
  - node: Improved GetActiveHandles performance (Trevor Norris)
nodejs#3780
* module: Errors during require() now provide more information (Brian
White) nodejs#4287
jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 24, 2015

jasnell commented Dec 24, 2015

Copy link
Copy Markdown
Member

Marking as semver-major due to the error handling change. It's not clear if this is major or minor tho. @nodejs/ctc

jasnell mentioned this pull request Mar 17, 2016
Closed
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
module: always decorate thrown errors
65c424f 
This provides more information when encountering a syntax or similar
error when executing a file with require().

Fixes: nodejs#4286
PR-URL: nodejs#4287
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@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

module Issues and PRs related to the module subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL