-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
breaking(middleware/devtools): use official devtools extension types #819
breaking(middleware/devtools): use official devtools extension types #819
Conversation
Size Change: -384 B (-1%) Total Size: 29.7 kB
ℹ️ View Unchanged
|
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.
@devanshj Hi, can you check this please?
symbol?: boolean | ||
map?: boolean | ||
set?: boolean | ||
} |
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.
So, this is breaking. Why did we have options
in the first place?
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.
Not sure why there was the options
thing but this is a good change. We can deprecate the existing shape and support both shapes old and new in 3.x
. (Like detect with serialize.options !== undefined
or something and have another overload for the new options to add the deprecation jsdoc hint on the old ones)
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.
nice idea. i'll try.
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.
extensionConnector = | ||
(window as any).__REDUX_DEVTOOLS_EXTENSION__ || | ||
(window as any).top.__REDUX_DEVTOOLS_EXTENSION__ | ||
extensionConnector = window.__REDUX_DEVTOOLS_EXTENSION__ |
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.
Not sure why we need top
. The official '@redux-devtools/extension' doesn't seem to have one.
This is breaking too.
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.
Probably so that the devtools work in codesandbox etc (as __REDUX_DEVTOOLS_EXTENSION__
would only be in the parent window)
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.
hmm, let me experiment. (jotai/valtio doesn't do this, so we want to make three consistent.)
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.
https://codesandbox.io/s/cool-sun-bnowv5
window.top
doesn't work anyway in codesandbox.
(can't comment on an unchanged line) - let extension = Object.create(extensionConnector.connect(devtoolsOptions))
+ let extension = (Object.create as <T>(t: T) => T)(extensionConnector.connect(devtoolsOptions))
|
src/middleware/devtools.ts
Outdated
@@ -27,21 +29,19 @@ type StoreSetStateWithAction<S> = S extends { getState: () => infer T } | |||
interface DevtoolsOptions { |
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.
See if we can use the exported EnhancerOptions
type too (We'd also get the jsdoc for free). Something like...
interface DevtoolsOptions extends Pick<EnhancerOptions, "name" | "serialize"> {
anonymousActionType?: string
}
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.
This works, but now TS users have to install some devDependencies (very counter-intuitive), which isn't very handy. Let's keep it as is.
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.
Yeah that's fair. One could write a postinstall script that fetches the .d.ts
files from unpkg and writes it to the filesystem but I think that would be going too far.
src/middleware/devtools.ts
Outdated
@@ -1,3 +1,5 @@ | |||
import '@redux-devtools/extension' |
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.
Perhaps you'd know this better but just make sure this doesn't add any unused runtime code.
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.
yeah, the final app code should be fine, but we should actually remove in dist. I'll see how I can do it.
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.
e9cf15e this should do.
Thanks. 06123b1 Now, I get type errors... |
src/middleware/devtools.ts
Outdated
if (action.type === '__setState') { | ||
setStateFromDevtools(action.state as PartialState<S>) | ||
return | ||
;(extension as any) // FIXME no-any |
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.
It would be nice if we could use this type definition: https://github.com/reduxjs/redux-devtools/blob/b82de745928211cd9b7daa7a61b197ad9e11ec36/extension/src/app/api/index.ts#L522-L536
export interface ConnectResponse {
init: <S, A extends Action<unknown>>(
state: S,
liftedData?: LiftedState<S, A, unknown>
) => void;
subscribe: <S, A extends Action<unknown>>(
listener: (message: ListenerMessage<S, A>) => void
) => (() => void) | undefined;
unsubscribe: () => void;
send: <S, A extends Action<unknown>>(
action: A,
state: LiftedState<S, A, unknown>
) => void;
error: (payload: string) => void;
}
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.
Yeah. It's a little weird that ConnectResponse
in @redux-devtools/extension
only has init
and send
, one could consider opening an issue to write the actual type definition too.
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.
filed: reduxjs/redux-devtools#1097
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 0efb349:
|
Wouldn't this be more maintainable?