proposal-error-stacks icon indicating copy to clipboard operation
proposal-error-stacks copied to clipboard

Align paths in traces

Open piranna opened this issue 6 years ago • 13 comments

Initially proposed at https://github.com/nodejs/node/issues/22575, and cross-posting here as recommended at https://github.com/nodejs/node/issues/22575#issuecomment-511999714.

When an exception is thrown or when using console.trace(), files paths are not aligned making it difficult to follow them at naked eye and specially to identify when it's one of your files, one internal module of if it's a file located inside node_modules folder:

UnhandledPromiseRejectionWarning: SyntaxError: Identifier 'body' has already been declared
     at Test.Runnable (/opt/app/node_modules/mocha/lib/runnable.js:36:25)
     at new Test (/opt/app/node_modules/mocha/lib/test.js:24:12)
     at context.it.context.specify (/opt/app/node_modules/mocha/lib/interfaces/bdd.js:85:18)
     at Function.context.it.only (/opt/app/node_modules/mocha/lib/interfaces/bdd.js:96:46)
     at Object.<anonymous> (/opt/app/src/modules/templates/template.spec.js:21:4)
     at Module._compile (internal/modules/cjs/loader.js:689:30)
     at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
     at Module.load (internal/modules/cjs/loader.js:599:32)
     at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
     at Function.Module._load (internal/modules/cjs/loader.js:530:3)
     at Module.require (internal/modules/cjs/loader.js:637:17)
     at require (internal/modules/cjs/helpers.js:20:18)
     at /opt/app/node_modules/mocha/lib/mocha.js:250:27
     at Array.forEach (<anonymous>)
     at Mocha.loadFiles (/opt/app/node_modules/mocha/lib/mocha.js:247:14)
     at Mocha.run (/opt/app/node_modules/mocha/lib/mocha.js:576:10)

Not sure if this is done at v8 level, but my proposal is to add spaces between the function name and the file paths so this last ones gets aligned between themselves to the longest one. In the previous trace, it would get like:

UnhandledPromiseRejectionWarning: SyntaxError: Identifier 'body' has already been declared
     at Test.Runnable                 (/opt/app/node_modules/mocha/lib/runnable.js:36:25)
     at new Test                      (/opt/app/node_modules/mocha/lib/test.js:24:12)
     at context.it.context.specify    (/opt/app/node_modules/mocha/lib/interfaces/bdd.js:85:18)
     at Function.context.it.only      (/opt/app/node_modules/mocha/lib/interfaces/bdd.js:96:46)
     at Object.<anonymous>            (/opt/app/src/modules/templates/template.spec.js:21:4)
     at Module._compile               (internal/modules/cjs/loader.js:689:30)
     at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
     at Module.load                   (internal/modules/cjs/loader.js:599:32)
     at tryModuleLoad                 (internal/modules/cjs/loader.js:538:12)
     at Function.Module._load         (internal/modules/cjs/loader.js:530:3)
     at Module.require                (internal/modules/cjs/loader.js:637:17)
     at require                       (internal/modules/cjs/helpers.js:20:18)
     at                                /opt/app/node_modules/mocha/lib/mocha.js:250:27
     at Array.forEach                 (<anonymous>)
     at Mocha.loadFiles               (/opt/app/node_modules/mocha/lib/mocha.js:247:14)
     at Mocha.run                     (/opt/app/node_modules/mocha/lib/mocha.js:576:10)

There would be problems if some code is parsing the trace output taking in consideration to be just only a space between the function name and the file path instead of several spaces, so this change would need to be in a major version, but anyway exception traces are not standard and such libs are very few and mostly for debuging purposses so their impact will be low, and is a small change that they would be easily added.

piranna avatar Jul 16 '19 21:07 piranna

This proposal is currently only attempting to specify the structure, not the contents - but either way, that kind of alignment doesn't match what browsers/engines currently do, so you'd have to build your own function that used the structured data provided by this proposal in order to format the stack trace how you liked.

Alignment is a subjective thing; many people like it, but many people (myself included) find it much harder to read.

ljharb avatar Jul 16 '19 22:07 ljharb

that kind of alignment doesn't match what browsers/engines currently do, so you'd have to build your own function that used the structured data provided by this proposal in order to format the stack trace how you liked.

I was suggested to implement it in Node.js and open a pull-request for that, I'll try to find some time to do it.

Alignment is a subjective thing

Agree. In my case, I find it better to have them aligned so it's easier to identify the actual functions and their location at a glance, without needing to read/scan all the stack trace lines.

piranna avatar Jul 16 '19 22:07 piranna

That's a good reason to make it be a separate utility function, so that you have to opt in to this nonstandard formatting.

ljharb avatar Jul 16 '19 22:07 ljharb

A wrapper function that parses and format the stack trace? Yes, doable, but not a default behaviour when an error ocurrs, that's when you need it...

piranna avatar Jul 16 '19 22:07 piranna

Right - but since not everyone will prefer the same default as you, it doesn't make sense to ruin it for them and change the default. I think a wrapper function is the better approach, for node as well.

ljharb avatar Jul 16 '19 22:07 ljharb

Why not split the difference on this one? Add a formatting function to the spec. Default value can be undefined or null. If set, let the getter for Error.prototype.stack use it to format the stack entries. If it's not present, then use the engine default formatting.

rdking avatar Jun 22 '21 17:06 rdking

Or, if we want the paths aligned, we could just swap the order of paths and functions, that way it doesn't take up much horizontal space. It would be nice if it was easier to distinguish the stack frames from external libraries/npm modules, or even different parts of your own projects.

I guess the message shown to the end-user when an error actually goes uncaught can use a differently formatted stack trace than what's found in the "stack" property, but I doubt there would be much incentive to deviate from it.

And, I see the value in just trying to standardize what implementations are already doing, without forcing them to make changes, so while a different format would be nice, I don't really see that happening in this spec.

theScottyJam avatar Jun 22 '21 17:06 theScottyJam

Alignment is a subjective thing; many people like it,

And even people who like it (I do), need different kinds of whitespace in different kinds of projects, e.g.

  • just ascii space
  • full-width space
  • ascii space + non-breaking space
  • non-breaking space + ascii space
  • ascii space, dots, ascii space

mk-pmb avatar Jul 01 '21 21:07 mk-pmb

And even people who like it (I do), need different kinds of whitespace in different kinds of projects, e.g.

KISS principle: use ASCII whitespaces.

piranna avatar Jul 01 '21 21:07 piranna

How can we move this forward?

piranna avatar Mar 01 '22 23:03 piranna

It’s not clear there is a path forward here, until after this proposal has landed.

ljharb avatar Mar 01 '22 23:03 ljharb

until after this proposal has landed

Are you talking about my proposal of aligned stack traces, or the one of this repo about standarize them?

piranna avatar Mar 02 '22 07:03 piranna

This one. Until they’re standardized as-is, it won’t be possible for any proposal to enhance or change them.

ljharb avatar Mar 02 '22 08:03 ljharb