-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
Improve display of filenames in component stack #12059
Improve display of filenames in component stack #12059
Conversation
@@ -17,7 +17,7 @@ export default function( | |||
(name || 'Unknown') + | |||
(source | |||
? ' (at ' + | |||
source.fileName.replace(/^.*[\\\/]/, '') + | |||
source.fileName.match(/[^\\\/]*([\\\/]index\.[^\\\/]+)?$/)[0] + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment with different things it’s supposed to match?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :)
Can you add some tests ensuring this works? Maybe you could manually create element objects with |
Sorry for the delay @gaearon. I'll come back to this and add some tests as soon as I'm able to get a Yarn env set up. (If someone else wants to tackle this first, feel free to co-opt this PR!) |
Ping :-) |
Sorry, I've been traveling and don't want to mess with my env again until I have a block of spare time. Np if you want to close this until I come back to it. |
@billyjanitsch, I've created a working test case for your change - how would you like me to contribute to this PR :)? Create a PR to your forked repo or..? |
Thanks @raunofreiberg, much appreciated! I gave you push access to my fork so you can update the PR directly, if you'd like. |
Thanks @billyjanitsch. I've updated your PR, although the Danger tests seem to be failing again despite the #12295 fix. @gaearon I couldn't find a decent way to use the |
The tests are probably failing because my fork is based on an old master. Let me try rebasing. Update: 🎉 |
Ping @gaearon 😄 |
Ping everyone 😉 |
The only thing that holds me back from merging is I'm not confident this regex always works. :-) |
I didn't quite trust this regex so I rewrote with more comprehensive tests. Thank you for starting this! |
ReactDOM: size: 🔺+0.1%, gzip: 🔺+0.2% Details of bundled changes.Comparing: fa824d0...69f7c41 react
react-dom
react-art
react-test-renderer
react-reconciler
react-native-renderer
Generated by 🚫 dangerJS |
Um apparently this is PROD code path too? I forgot. I'll wrap this in a DEV block since it's non-critical. |
Fixes #12058, fixes #11519, and supercedes #11523.
FWIW, as a user, I really like the idea of only stripping the common head, but I understand that you want to start simple. :)
Relying on CI to run the test suite for now -- I tried to install Yarn but it clobbered my fnm installation. 😢