reduxjs / reselect

Selector library for Redux
MIT License
19.04k stars 670 forks source link

Better call stack for selector warnings #670

Open uncaught opened 10 months ago

uncaught commented 10 months ago

The call stack for the new dev mode checks starts/ends with useSelector. But if you have a lot of nested selectors, finding the one actually causing the warning can be a bit tedious.

Would it be possible to add the location of the actual createSelector-call to the warning?

E.g.:

//file a
const a = createSeletctor(...);

//file b
const b = createSeletctor(...); //causing the warning
const c = createSeletctor(...);

//file d
const d = createSeletctor(a, b, c, () => a+b+c /* not causing the warning */);

//file e
useSelector(d); //generates the warning, call stack states this line only

-> Please add "file b, line x" to the warning.

aryaemami59 commented 10 months ago

@uncaught Yeah I just looked at it again and you're right, the location where the selector is defined is not in the stack trace, I'm going to look into this. Thanks for opening the issue.

EskiMojo14 commented 9 months ago

I'm not convinced we can do much about this? we get the stack string by throwing and catching an Error, it's up to the platform what the stack actually looks like

uncaught commented 9 months ago

Well yes, but you don't need to hack the system's callstack to show something in the warning message.

Like if you determine the file and location of the createSelector call, store it and print it out with the warning.

markerikson commented 9 months ago

@uncaught the issue is that trying to parse the stack and come up with the filename and such is more complex and browser-dependent.

It's not impossible. But it would definitely take extra code and extra effort, and reading a stack trace is a core skill that we would expect end user devs to have.

uncaught commented 9 months ago

Haven't tried it, I'm on my phone, but this I've just found in a minute:

(new Error()).stack.split("\n")[1].split("/").slice(-1)[0]

Store this and print it in the error handler.

The issue here is that the call stack doesn't show the location of the faulty selector at all. This is not about being too lazy to read it.

EskiMojo14 commented 9 months ago

if it's not in the call stack, i don't see how we'd get that information for you?

uncaught commented 9 months ago

As i said, determine the file+line of the createSelector call, which should be possible, see above. Then store that file and line number somewhere. For example put it on the resolver function as a Smybol or create a Map for it or whatever.

Then when you print that warning, look up the file an number for the selector that caused the warning situation and print it.

markerikson commented 9 months ago

We're open to PRs here, but the current system does show useful information as-is and this isn't an immediate priority for us.

uncaught commented 9 months ago

I thought @aryaemami59 was already looking into it (first comment).

But sure, this isn't a high priority thing, just an inconvenience.

EskiMojo14 commented 9 months ago

@uncaught I don't think we're going to try and parse the stack, for the reasons Mark stated.

With that said, it's doable to create a stack when you first call createSelector - do you fancy trying the build from https://github.com/reduxjs/reselect/pull/684 and seeing if that includes the information you'd want?

aryaemami59 commented 9 months ago

@uncaught sorry I got busy with other things, I did play around with it some time ago, it seemed like it was going to be more challenging than I anticipated, so it was in my todo list just didn't get around to dealing with it yet. And it seems like @EskiMojo14 is going to give it a shot, he is much more quialified to look at this than I am anyway.