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

Show stack traces #43

Merged
merged 15 commits into from
Dec 18, 2018
Merged

Conversation

markerikson
Copy link
Contributor

When the underlying library is Redux, a "Stack" tab is now added after the "Diff" tab. When selected, it deserializes the Error that was captured when the original action was dispatched, formats the stack trace using stacktrace-js, and renders the resulting stack frames.

This is a bare-bones implementation, and does not actually format the stack frame list beyond stringifying each frame entry. It also isn't filtering the list of stack frames at all, so there's frames from both the instrumentation and React in there.

Ideally, it would be great if we could filter out any frames that aren't part of the application source, and highlight the specific frame that actually triggered the dispatching sequence. But, this is a good first step - the data is being passed around, and we can do more formatting later.

See zalmoxisus/redux-devtools-extension#429 for the original request.

@markerikson
Copy link
Contributor Author

I've just pushed a sizeable update to this branch, which reuses the stack parsing logic and stack trace UI components from react-error-overlay to show the action stack trace in a much more readable format. It also at least tries to call a browser API to show the associated file when a stack entry is clicked, although that is somewhat unreliable atm due to mismatches between sourcemapped and original filenames.

However, the branch now has some issues:

  • The code I copied from react-error-overlay completely fails the lint checks
  • It also uses async/await, which the current Babel config for this project doesn't handle.
  • The react-error-overlay UI color scheme doesn't fit well into the DevTools UI look and feel. (It's hardcoded to be whites, yellows, and reds).

The linting stuff can be fixed one way or another. My immediate concern is the Babel config.

To get this to work, I updated the config to use babel-preset-env targeting some recent browser versions. I realize that isn't a good solution long-term, but I just wanted to get this running at all.

I'd be happy to discuss the best way to get this fixed up and merged in ASAP. Please let me know what would be a good way to handle this.

@zalmoxisus
Copy link
Owner

Thanks @markerikson. I'll test it and publish the extension tomorrow. Regarding the ui, you might give a try to new-ui branch. I'll try to move that to the extension, at least to cut an alpha, so others can easier review.

@zalmoxisus
Copy link
Owner

zalmoxisus commented Nov 13, 2018

@markerikson would you like to move it as a separate monitor and maintain it? See redux-devtools-test-generator, in that way it would work also in vanilla redux-devtools, not only via remotedev-app or extension. Also see there on how to add an unified UI via devui.

If you don't have time these days to work on that, I could merge it as is, but it won't be enabled by default, will add a message on how to enable it from client part.

@zalmoxisus
Copy link
Owner

I published remotedev-app@0.10.9-alpha, but unfortunately wasn't able to use it with extension. First error-stack-parser package is missing, which you import in StackTraceTab.js. Second you have ES6 code, so UglifyJs doesn't work (but we need that to minify the code, otherwise cannot upload the extension to Mozilla Addons). Babel should transform that code into ES5 when building lib folder.

When there's no callstack, we have only Dispatched Action Stack Trace header (on white background). Would be great to check that and show in that case a message (keeping default background and color in this case) like: Please add shouldIncludeCallstack parameter if you want to propagate dispatched action callstack.. Ideally that tab should be visible only for PERFORM_ACTION as monitor actions don't need callstack (and we don't send them from redux-devtools-instrument).

Also please remove the code which is not meant for production, I see some console.log, and buildAndCopy added in package.json is only for Windows and it wouldn't work anyway as we also need dependences. There's a lot to fix in linting, so if you want to move it later as a separate monitor and use your linting rules there, we could skip that for now.

zalmoxisus added a commit to reduxjs/redux-devtools that referenced this pull request Dec 12, 2018
zalmoxisus added a commit to reduxjs/redux-devtools that referenced this pull request Dec 13, 2018
@zalmoxisus zalmoxisus mentioned this pull request Dec 13, 2018
7 tasks
@zalmoxisus
Copy link
Owner

@markerikson I moved the code to a separate package in reduxjs/redux-devtools#418 and added some tweaks. Mentioned you as the author and added credits in the README. Feel free to send a PR there with changes if any.

I'll add tomorrow more details about how to check it in the related issue.

@markerikson
Copy link
Contributor Author

Cool. Yeah, I've been really busy with Redux docs related stuff over the last couple months, so I haven't had time to think about this again. Thanks for poking at it :)

@markerikson
Copy link
Contributor Author

Quick question: is this going to be enabled by default in the extension and instrumentation, or will most users need to do something to turn it on?

@zalmoxisus
Copy link
Owner

zalmoxisus commented Dec 15, 2018

Quick question: is this going to be enabled by default in the extension and instrumentation, or will most users need to do something to turn it on?

Yes, it will be present on the extension / remotedev-app, but it should be enabled on the client side (like in the examples), and if necessary limiting the number of frames (not a problem in Chrome since it's limited to 10 on browser side, but could be on Firefox). We might enable by default in future if it won't impact the performance significantly. The problem is that most are including the extension and not using it, so it will affect the perf when not needed. I guess we'll just show a suggestion in that tab to set that option if no trace stack received. Also the editor name and project path should be added in the extension's options.

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

Successfully merging this pull request may close these issues.

2 participants