-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Stack trace for act warning #434
Comments
I think React is the right place for this kind of fix. Let's see if we can get them to do that. It's a great idea! |
@kentcdodds Looks like they don't want to do anything about it and honestly, I don't like the approach of requiring to configure Babel plugin. Would you reconsider we would include my "hack" in here? That would work without any extra configuration. |
I don't think that Dan was suggesting that you use the babel plugin in tests. I think he was just saying that's how it works in the browser. I agree with Dan, that this is something which would be really cool to have built-into Jest. Here's a proof of concept of what I mean (and what I think Dan is suggesting): const originalError = console.error
beforeAll(() => {
console.error = (...args) => {
console.log(new Error().stack)
originalError(...args)
}
})
afterAll(() => {
console.error = originalError
}) This would look like:
And visually: And it could be potentially improved within Jest so it's even more helpful (with color coding to highlight the part of the code that triggered the error). Similar to when an error is thrown.
And visually: To be clear, what I mean is that I think this would help more than just |
I like this so much I'll go ahead and file the issue myself. |
Opened: jestjs/jest#8819 |
Not exactly, that plugin is included in CRA and that's why it has proper line codes for affected components, in tests. It has nothing to do with the browser. I agree it would be super nice to have it the way you outlined above but consider it would be for Jest users only. I've seen cases of people running away from Jest because it tends to get slow. It would be really best if React would include it, but I still think that RTL is kinda the second-best place. RTL mission is to make testing easier, right? So I wonder, how is that hack of mine not fulfilling that mission? Even if it would be optional in a style of requiring script in setupTests.js. Or even if it would be temporary till a better solution is crafted. It would help many people now, I am sure if of it. For a reference, lately, I have spotted these two cases lately where people are obviously struggling with finding why is act warning shown. https://spectrum.chat/testing-library/help-react/act-warning~be946bdd-b2c4-4bf5-bf4d-e4a1f01d1914 |
I don't disagree with you that it is a problem. We're just working out the best place for the solution to be. Please be patient. |
Don't worry about me, I am using that hack file by myself, there is nothing spectacular about it. I am merely thinking about other people who are stumbling in the dark. By adding that hack into RTL it could temporarily help until a proper solution is made. I am willing to do PR, just give the green light :) |
Don't worry. I know that this is a problem. |
Describe the feature you'd like:
I've already filled the issue facebook/react#16348, but then it occurred to me that RTL might be perhaps a better candidate for such utility. If anything, it will be surely published faster :)
In the following sandbox, I've figured out a way how to actually get a stack trace for the originating location of state change that's missing
act
wrapper.Teachability, Documentation, Adoption, Migration Strategy:
Considering it won't be swallowing original warning, it should be safe to enable it by default for everyone.
The text was updated successfully, but these errors were encountered: