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

fix: handle anonymous callsites. (fixes #5) #30

Merged

Conversation

jordrake
Copy link
Contributor

Fixes #5 by ensuring we only call path.basename on callsites with a valid file path. The find below should act as a filter of these falsy (usually null from what I've seen) values.

In the unlikely case that there is no suitable file path, set the filename as unknown.

@jordrake jordrake force-pushed the handle-anonymous-callsites branch from babf5d9 to 23bd605 Compare May 30, 2018 19:48
@klaudiosinani klaudiosinani self-requested a review May 30, 2018 23:11
@klaudiosinani klaudiosinani added the enhancement New feature or request label May 30, 2018
signale.js Outdated
@@ -62,7 +62,7 @@ class Signale {
const {stack} = new Error();
Error.prepareStackTrace = _;

const callers = stack.map(x => path.basename(x.getFileName()));
const callers = stack.map(x => x.getFileName() && path.basename(x.getFileName()));
Copy link
Owner

@klaudiosinani klaudiosinani May 30, 2018

Choose a reason for hiding this comment

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

By replacing the lines 65-66 of signale.js with the following:

// signale.js 
// ...
const callers = stack.map(x => x.getFileName()); // L65
console.log(callers); // L66
// ...

and then running example of #5 on both node 8.9.1 and 9.11.1:

// file path: home/signale/foo.js
const signale = require('signale');
signale.config({displayFilename: true});

Promise.resolve().then(() => {
  signale.error('Foo');
});

we get these file paths, returned by the callsites:

// Filepaths returned by callsites
[ 'home/signale/foo.js',
  'home/signale/foo.js',
  'home/signale/foo.js',
  'home/signale/foo.js',
  'home/signale/foo.js',
  'home/signale/foo.js',
  null,
  'internal/process/next_tick.js',
  'module.js',
  'bootstrap_node.js' ]

Thus, the issue is indeed path.basename(x.getFileName()), that crushes once null, returned by the <anonymous> callsite, is pass to it as an argument. So, instead of returning unknown, we can still return the filename foo.js just by calling path.basename at the end of filename(), e.g:

  get filename() {
    const _ = Error.prepareStackTrace;
    Error.prepareStackTrace = (error, stack) => stack;
    const {stack} = new Error();
    Error.prepareStackTrace = _;

    const callers = stack.map(x => x.getFileName()); // Remove path.basename()

    const filePath = callers.find(x => {  // Get the whole filepath
      return x !== callers[0];
    });

    return path.basename(filePath);  // Return the basename
  }

It is an initial idea, so please feel free to share your thoughts : )

Copy link
Contributor Author

@jordrake jordrake May 31, 2018

Choose a reason for hiding this comment

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

The unknown case was to handle a scenario where we had so many anonymous call-sites that it pushed any sensible filename past the callsite limit (default I believe is 10) where get filename would have a stack trace like:

Error
    at Signale.get filename [as filename] (/mnt/d/dev/signale/signale.js:65:17)
    at Signale._formatFilename (/mnt/d/dev/signale/signale.js:100:21)
    at Signale._meta (/mnt/d/dev/signale/signale.js:124:22)
    at Signale._buildSignale (/mnt/d/dev/signale/signale.js:151:26)
    at Signale._logger (/mnt/d/dev/signale/signale.js:82:20)
    at <anonymous>
    at <anonymous>
	at <anonymous>
	at <anonymous>
    at <anonymous>
	at <anonymous>
	at <anonymous>
    at <anonymous>
	at <anonymous>

I've not actually managed to create this scenario mind you. Happy to remove it.

Copy link
Owner

@klaudiosinani klaudiosinani May 31, 2018

Choose a reason for hiding this comment

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

I completely agree, we can then change the return statement to something like this:

return firstExternalFilePath ? path.basename(firstExternalFilePath) : 'anonymous';

This way even the most extreme cases can be covered and also avoid any potential errors on the way

@jordrake jordrake force-pushed the handle-anonymous-callsites branch from 23bd605 to e49b186 Compare May 31, 2018 09:25
signale.js Outdated
return x !== callers[0];
});

return path.basename(firstExternalFilePath);
Copy link
Owner

Choose a reason for hiding this comment

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

Original comment: #30 (comment)

return firstExternalFilePath ? path.basename(firstExternalFilePath) : 'anonymous';

@jordrake jordrake force-pushed the handle-anonymous-callsites branch from e49b186 to ef1401d Compare May 31, 2018 14:16
@klaudiosinani
Copy link
Owner

klaudiosinani commented Jun 1, 2018

Awesome! Thank you a lot : )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants