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

Better call stack for selector warnings #670

Open
uncaught opened this issue Dec 18, 2023 · 11 comments
Open

Better call stack for selector warnings #670

uncaught opened this issue Dec 18, 2023 · 11 comments

Comments

@uncaught
Copy link

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
Copy link
Contributor

@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
Copy link
Contributor

EskiMojo14 commented Jan 15, 2024

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
Copy link
Author

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
Copy link
Contributor

@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
Copy link
Author

uncaught commented Jan 15, 2024

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
Copy link
Contributor

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

@uncaught
Copy link
Author

uncaught commented Jan 15, 2024

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
Copy link
Contributor

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
Copy link
Author

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

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

@EskiMojo14
Copy link
Contributor

@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 #684 and seeing if that includes the information you'd want?

@aryaemami59
Copy link
Contributor

@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.

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

No branches or pull requests

4 participants