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

Add trace property to devtools options #752

Closed
wants to merge 1 commit into from
Closed

Add trace property to devtools options #752

wants to merge 1 commit into from

Conversation

DanielPower
Copy link
Contributor

@DanielPower DanielPower commented Jan 13, 2022

Relates to #716

From my testing, redux dev tools trace feature is already working without any changes on Zustand's side. All that's necessary is to type the option so that Typescript will allow it to be passed.

There is at least one caveat, and that is that if you try to use a callback for trace, it does not correctly get passed the action as an argument. This functionality exists so you can either customize the trace, or to conditionally trace on some actions like in the example below.

trace: (action) => {
  if (shouldTrace(action)) {
    return new Error().trace;
  }
},

In this example, action will always come back as undefined. I intend to look into this and will put up another PR when I can with the necessary fixes. However as an initial step, I'm putting up this PR to type the field, because much of the functionality is already working.

Edit: I've also gone ahead and added traceLimit and features, as I've tested both of those as working. I'll type out more features in the future as I test them.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 13, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 6bd6174:

Sandbox Source
React Configuration
React Typescript Configuration
React Browserify Configuration
React Snowpack Configuration
React Parcel Configuration
Next.js Configuration

@DanielPower DanielPower marked this pull request as draft January 13, 2022 03:33
@DanielPower DanielPower marked this pull request as ready for review January 13, 2022 03:41
Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this.

src/middleware/devtools.ts Outdated Show resolved Hide resolved
Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM unless @devanshj shows any concern.

Comment on lines +21 to +32
features?: {
pause?: boolean
lock?: boolean
persist?: boolean
export?: boolean | 'custom'
import?: boolean | 'custom'
jump?: boolean
skip?: boolean
reorder?: boolean
dispatch?: boolean
test?: boolean
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure we don't need any changes in our implementation to support these flags? Are these entirely handled by the extension? And could point to the type definition of this from the repo. We should also include jsdoc for them.

You can opt to remove features altogether, no issues, I just don't want to do things half-baked :)

dispatch?: boolean
test?: boolean
}
trace?: boolean | (() => string)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove (() => string) if it's not working. (I assume we need to change our implementation to support it?)

Comment on lines +33 to +34
trace?: boolean | (() => string)
traceLimit?: number
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following applies to trace and traceLimit too

Are you sure we don't need any changes in our implementation to support these flags? Are these entirely handled by the extension? And could point to the type definition of this from the repo. We should also include jsdoc for them.

@@ -1,5 +1,39 @@
import { GetState, PartialState, SetState, State, StoreApi } from '../vanilla'

type DevtoolsOptions = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're interested you can look up the repo, find out what other options we're missing that don't require implementation changes and add jsdocs for the them and the existing ones too

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider @devanshj's comments.

@DanielPower
Copy link
Contributor Author

DanielPower commented Jan 24, 2022

Thank you for the comments. Will loop back to this as soon as I can.

Edit: Sorry for the delay. I've been pulled away from global state for a bit. I'm still planning to look into this, but it's going to be 2 weeks before I'll be able to do it.

@dai-shi
Copy link
Member

dai-shi commented Apr 18, 2022

Closing this as stale. Please start over because we have many changes in the main branch. #716 should be the issue for tracking and discussion.

@dai-shi dai-shi closed this Apr 18, 2022
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.

3 participants