supports: atom toString includes debugLabel in dev mode #156
Conversation
|
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. |
dmaskasky
force-pushed
the
atom-tostring
branch
2 times, most recently
from
3b1cd8e
to
c11a1bf
Compare
src/utils/useAtomsDevtools.ts
Outdated
| @@ -8,8 +8,13 @@ import { getReduxExtension } from './redux-extension/getReduxExtension'; | |||
| import { SnapshotOptions, useAtomsSnapshot } from './useAtomsSnapshot'; | |||
| import { useGotoAtomsSnapshot } from './useGotoAtomsSnapshot'; | |||
|
|
|||
| const atomToPrintable = (atom: AnyAtom) => | |||
| atom.debugLabel ? `${atom}:${atom.debugLabel}` : `${atom}`; | |||
| const atomToPrintable = (atom: AnyAtom) => { | |||
There was a problem hiding this comment.
We have copies of this function across the codebase, could you pls update those too or perhaps move it to a shared function somewhere in the internals?
https://github.com/search?q=repo%3Ajotaijs%2Fjotai-devtools%20atomToPrintable&type=code
There was a problem hiding this comment.
or perhaps move it to a shared function somewhere in the internals?
I think I like this option more.
There was a problem hiding this comment.
https://github.com/search?q=repo%3Ajotaijs%2Fjotai-devtools%20atomToPrintable&type=code
These are only the ones named as such. There may be more.
There was a problem hiding this comment.
Good point! There is one more to update then 😄
There was a problem hiding this comment.
I reviewed this list. I actually don't think any further changes are necessary.
- src/utils/useAtomsDevtools.ts#L11 - Changed in this PR
const atomToPrintable = (atom: AnyAtom) =>
atom.debugLabel ? `${atom}:${atom.debugLabel}` : `${atom}`;- src/utils/useAtomsDebugValue.ts#L13 - Only outputs debugLabel, so not the same logic as src/utils/useAtomsDevtools.ts#L11
const atomToPrintable = (atom: Atom<unknown>) =>
atom.debugLabel || atom.toString();- src/DevTools/utils/atom-to-printable.ts#L9 - Only outputs debugLabel, so not the same logic as src/utils/useAtomsDevtools.ts#L11
export const atomToPrintable = (atom: AnyAtom): string => {
return atom.debugLabel ? atom.debugLabel : `<unlabeled-${atom}>`;
};...Rest are references to 1, 2, and 3 above.
There was a problem hiding this comment.
Good point! There is one more to update then 😄
- src/utils/useAtomDevtools.ts#L29 - Same concern. Only outputs debugLabel, so not the same logic as src/utils/useAtomsDevtools.ts#L11
const atomName = name || anAtom.debugLabel || anAtom.toString();There was a problem hiding this comment.
@arjunvegda
I agree this logic should be centralized, but can we move centralizing print logic to another PR since it will change the underlying logic?
This PR is to accommodate pmndrs/jotai#2659 in jotai core so that it doesn't break devtools.
There was a problem hiding this comment.
There was a problem hiding this comment.
You're right! we can do a broader overhaul of how these labels are used in dev tools in other PRs.
src/utils/useAtomsDevtools.ts
Outdated
| @@ -8,8 +8,13 @@ import { getReduxExtension } from './redux-extension/getReduxExtension'; | |||
| import { SnapshotOptions, useAtomsSnapshot } from './useAtomsSnapshot'; | |||
| import { useGotoAtomsSnapshot } from './useGotoAtomsSnapshot'; | |||
|
|
|||
| const atomToPrintable = (atom: AnyAtom) => | |||
| atom.debugLabel ? `${atom}:${atom.debugLabel}` : `${atom}`; | |||
| const atomToPrintable = (atom: AnyAtom) => { | |||
There was a problem hiding this comment.
Any thoughts on updating the fallback to something like <unlabeled-${atom}> 🤔. We do it for the UI version but not really for the Redux and React DevTools version
There was a problem hiding this comment.
I like it, but let's move this logic change / centralization to a separate PR.
Yes it does. I'll go back to that one. |
Related:
pmndrs/jotai#2659