module: fix error first line col offset by tflanagan · Pull Request #2867 · nodejs/node · GitHub

Rehash of nodejs/node-v0.x-archive#25342 with noted corrections.

Fixes #2860, nodejs/node-v0.x-archive#9445

I've also created another variant of this solution by adding a "\n" to the wrapper and setting the lineOffset property to -1, re: tflanagan@9d8e6a9

I feel like @brendan0powers's authorship should be preserved here.

👍

Just looking for things to do (this seemed rather important)

Are there any side effects of using the second commit (tflanagan@9d8e6a9)? It seems way simpler.

Need to also update docs on Script constructor

I've updated the docs in two places with the lineOffset and columnOffset parameters, and also added a missing filename and timeout parameter listings.

I agree with @brendanashworth that the other commit looks much simpler. Does it cause any issues on Windows?

bump for LTS? Don't want this PR to end up like @brendan0powers's

@tflanagan does tflanagan@489b5b4 cause any issues (perhaps windows)? I think this should be pushed instead of the other bigger commits. But this would be a candidate for LTS back-porting anyways.

I've beening running it on windows for over 3 weeks without issues, granted any issue would only show if an error occured

@tflanagan could you push only that commit to this PR then? Then we can review and run the CI.

@brendanashworth okay, i've rebased/amended/pushed it down to one commit with the NL approach.

$ vcbuild test nosign passed all tests

poke

edit: poke

The first line of this comment can probably be removed.

@tflanagan I'm interested in getting this landed. Would you mind rebasing?

@cjihrig Rebased, running new tests now, will force push shortly

@cjihrig Done.

Besides for those pesky flaky Buffer tests, it passes local tests fwiw

Edit: Fixed the docs

Thanks. LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/800/

@cjihrig Looks like it passed! :) 💯

💚 from the CI. Nice. Landing.

Because Node modules are wrapped, errors on the first line of a file leak the wrapper to the user and report the wrong column number. This commit adds a line break to the module wrapper so that the first line is treated the same as all other lines. To compensate for the additional line, a line offset of -1 is also applied to errors. Fixes: #2860 PR-URL: #2867 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Thanks! Landed in dfee4e3

Thank you! 👍

Because Node modules are wrapped, errors on the first line of a file leak the wrapper to the user and report the wrong column number. This commit adds a line break to the module wrapper so that the first line is treated the same as all other lines. To compensate for the additional line, a line offset of -1 is also applied to errors. Fixes: #2860 PR-URL: #2867 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

In dfee4e3, the module wrapper and line offset used when wrapping module code was changed to better report errors on the first line of modules. However, that commit did not update the runInThisContext() call used to execute the core modules, so their error line numbers have been off by one. This commit provides the correct lineOffset for core modules. Refs: nodejs#2867 PR-URL: nodejs#4254 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>

In dfee4e3, the module wrapper and line offset used when wrapping module code was changed to better report errors on the first line of modules. However, that commit did not update the runInThisContext() call used to execute the core modules, so their error line numbers have been off by one. This commit provides the correct lineOffset for core modules. Refs: #2867 PR-URL: #4254 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <jmwsoft@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Because Node modules are wrapped, errors on the first line of a file leak the wrapper to the user and report the wrong column number. This commit adds a line break to the module wrapper so that the first line is treated the same as all other lines. To compensate for the additional line, a line offset of -1 is also applied to errors. Fixes: #2860 PR-URL: #2867 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

In dfee4e3, the module wrapper and line offset used when wrapping module code was changed to better report errors on the first line of modules. However, that commit did not update the runInThisContext() call used to execute the core modules, so their error line numbers have been off by one. This commit provides the correct lineOffset for core modules. Refs: #2867 PR-URL: #4254 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <jmwsoft@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Because Node modules are wrapped, errors on the first line of a file leak the wrapper to the user and report the wrong column number. This commit adds a line break to the module wrapper so that the first line is treated the same as all other lines. To compensate for the additional line, a line offset of -1 is also applied to errors. Fixes: #2860 PR-URL: #2867 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

In dfee4e3, the module wrapper and line offset used when wrapping module code was changed to better report errors on the first line of modules. However, that commit did not update the runInThisContext() call used to execute the core modules, so their error line numbers have been off by one. This commit provides the correct lineOffset for core modules. Refs: #2867 PR-URL: #4254 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <jmwsoft@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

In dfee4e3, the module wrapper and line offset used when wrapping module code was changed to better report errors on the first line of modules. However, that commit did not update the runInThisContext() call used to execute the core modules, so their error line numbers have been off by one. This commit provides the correct lineOffset for core modules. Refs: nodejs#2867 PR-URL: nodejs#4254 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <jmwsoft@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

tflanagan force-pushed the module-fix-error-offsets branch from 76c8ac5 to 9c0f78b Compare September 14, 2015 22:40

brendanashworth mentioned this pull request Sep 14, 2015

module: fix column offset on first line stack trace nodejs/node-v0.x-archive#25342

Closed

brendanashworth added the module Issues and PRs related to the module subsystem. label Sep 14, 2015

tflanagan mentioned this pull request Sep 14, 2015

Error stack gives incorrect column number #2860

Closed

domenic reviewed Sep 15, 2015
View reviewed changes

tflanagan force-pushed the module-fix-error-offsets branch from 489b5b4 to 41810b8 Compare October 12, 2015 13:54

cjihrig mentioned this pull request Nov 16, 2015

require(): no information about where error #3784

Closed

cjihrig reviewed Nov 20, 2015
View reviewed changes

tflanagan force-pushed the module-fix-error-offsets branch from 41810b8 to 7dc621b Compare November 20, 2015 19:53

tflanagan force-pushed the module-fix-error-offsets branch 2 times, most recently from b28d69a to 1505c73 Compare November 20, 2015 19:57

module: fix error first line col offset

e4281ca

tflanagan force-pushed the module-fix-error-offsets branch from 1505c73 to e4281ca Compare November 20, 2015 19:58

cjihrig closed this Nov 20, 2015

tflanagan deleted the module-fix-error-offsets branch November 20, 2015 21:43

Fishrock123 mentioned this pull request Nov 26, 2015

deps: upgrade npm to 3.5.0 #4032

Closed

This was referenced Dec 8, 2015

Release proposal: v5.2.0 #4180

Closed

Release proposal: v5.2.0 (Stable) #4181

Merged

cjihrig mentioned this pull request Dec 12, 2015

src: fix line numbers on core errors #4254

Closed

ofrobots mentioned this pull request Dec 15, 2015

Debuggers & profilers broken on Node.js 5.2 #4297

Closed

rvagg mentioned this pull request Dec 17, 2015

Release 4.2.4 Planning #4321

Closed

cjihrig mentioned this pull request Dec 17, 2015

Fix debugger and profiler functionality #4298

Closed

jasnell added the lts-watch-v4.x label Dec 17, 2015

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Dec 29, 2015

MylesBorins mentioned this pull request Jan 19, 2016

V4.2.5 proposal #4768

Merged

module: fix error first line col offset by tflanagan · Pull Request #2867 · nodejs/node · GitHub

Uh oh!

Uh oh!

module: fix error first line col offset - #2867

module: fix error first line col offset#2867tflanagan wants to merge 1 commit intonodejs:masternodejs/node:masterfrom tflanagan:module-fix-error-offsetstflanagan/node:module-fix-error-offsetsCopy head branch name to clipboard

Conversation

tflanagan commented Sep 14, 2015

Uh oh!

kenany commented Sep 14, 2015

Uh oh!

tflanagan commented Sep 14, 2015

Uh oh!

brendanashworth commented Sep 15, 2015

Uh oh!

domenic Sep 15, 2015

Choose a reason for hiding this comment

Uh oh!

tflanagan Sep 15, 2015

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Sep 16, 2015

Uh oh!

tflanagan commented Oct 8, 2015

Uh oh!

brendanashworth commented Oct 10, 2015

Uh oh!

tflanagan commented Oct 10, 2015

Uh oh!

brendanashworth commented Oct 10, 2015

Uh oh!

tflanagan commented Oct 12, 2015

Uh oh!

tflanagan commented Oct 20, 2015

Uh oh!

cjihrig Nov 20, 2015

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Nov 20, 2015

Uh oh!

tflanagan commented Nov 20, 2015

Uh oh!

tflanagan commented Nov 20, 2015

Uh oh!

cjihrig commented Nov 20, 2015

Uh oh!

tflanagan commented Nov 20, 2015

Uh oh!

cjihrig commented Nov 20, 2015

Uh oh!

cjihrig commented Nov 20, 2015

Uh oh!

tflanagan commented Nov 20, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

module: fix error first line col offset#2867
tflanagan wants to merge 1 commit into
nodejs:masterfrom
tflanagan:module-fix-error-offsets