[internal] Rename useEventCallback and useLatestRef
#2987
Conversation
atomiks
added
the
internal
atomiks
force-pushed
the
refactor/untracked-hooks
branch
from
5327a8c to
851e845
Compare
commit: |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Bundle size report
|
romgrk
added
the
breaking change
| /** | ||
| * Stabilizes the function passed so it's always the same between renders. | ||
| * | ||
| * The function becomes non-reactive to any values it captures. | ||
| * It can safely be passed as a dependency of `React.useMemo` and `React.useEffect` without re-triggering them if its captured values change. | ||
| * | ||
| * The function must only be called inside effects and event handlers, never during render (which throws an error). | ||
| * | ||
| * This hook is a more permissive version of React 19.2's `React.useEffectEvent` in that it can be passed through contexts and called in event handler props, not just effects. | ||
| */ | ||
| export function useStableCallback<T extends Callback>(callback: T | undefined): T { |
There was a problem hiding this comment.
useStableCallback JSDoc
| /** | ||
| * Untracks the provided value by turning it into a ref to remove its reactivity. | ||
| * | ||
| * Used to access the passed value inside `React.useEffect` without causing the effect to re-run when the value changes. | ||
| */ | ||
| export function useValueAsRef<T>(value: T) { |
There was a problem hiding this comment.
useValueAsRef JSDoc
atomiks
force-pushed
the
refactor/untracked-hooks
branch
from
10ce18e to
a571d4b
Compare
github-actions
bot
added
the
PR: out-of-date
Signed-off-by: atomiks <[email protected]>
github-actions
bot
removed
the
PR: out-of-date
|
@romgrk the utils package is released together with the main |
|
No, the utils package is meant to be shared org-wide and decoupled from BUI. The goal is to avoid the problems of coupled versions that we had with I see that its version is currently at |
Following your statement and our own guidelines:
But what breaking change do you need to make that can't happen in a backwards compatible way until the next major? (e.g. keep old export but mark as I'm also happy to change or amend the guidelines if you have suggestions or a particular use-case that can't be sufficiently solved under them. I'm not sure it's the right strategy to fork major versions for the same npm scope. We all know how confusing that has been between X and core. cc @oliviertassinari for visibility |
I don't like that
Which next major? The utils package isn't synced to anything right now. BUI is at |
For the record, I don't advocate for making it
I think one advantage to sync major releases per npm scope is to signal end-users which packages are compatible with each other. e.g. similar to all babel packages sync their major release. Another advantage is that dependants of the utils package will also be dependants of BUI itself. It will be helpful to those packages if they can make sure their dependency on utils and the BUI dependency on utils share the same instance. It could be important for deduplication. And it will become a hard requirement if utils gain singleton behavior, or begin to share react contexts. Having a synced release on major version with a loose dependency range is the most reliable way to guarantee it dedupes correctly during end-user installation.
Semver is a mechanism that expresses compatibility between different versions of the same library. We also need to think about compatibility between different libraries that have to integrate. The answer needs more nuance. For a shared react support library I wouldn't necessarily recommend a workflow that make it easy to have multiple instances installed of the same library. |
We've already gone forward with an unsynced version, so we'll keep it as is for now, but let's make the upcoming release
But this isn't babel or even material-ui, the utils package in BUI is meant to be an org-wide generic utils package, it's only in BUI because we needed somewhere to place it. The utils package is a dependency of everything else, decoupled from any particular library.
Duplication would happen with your strategy anyway (keeping old code with |
You could almost argue it should be renamed and moved to
Not really, except perhaps during one patch version where dependants haven't had the chance to move to the new API yet. After that, tree-shaking removes the deprecated APIs. Nonetheless, the real harm comes from duplicate installations, not from having a shim around that allows usage of the old API until next major. |
Having the utils in a separate repo from MUI X is already somewhat painful as changes needed there require a PR and a release in BUI. In fact we have BUI code duplicated at many locations there due to the poor DX. It would make the DX even less convenient if there was yet another repo to consider. And tbh making breaking changes in the utils is something I appreciate, because it forces the rest of the codebases to evolve to better and more efficient patterns or hooks rather than stay on outdated code. For example, I've recently been reviewing the charts performance and because they stayed on an older (deprecated) version of the Store/selectors API, they ended up with lower performance and more refactoring work to do. |
I think I disagree with this paradigm.:
|
|
We already understand the pain points of a sync'ed utils package with |
Sure. to note that we do understand the pain points since releases were unsynced before we synced them. Also to note that under what I proposed, renaming the package would suffice to adhere to the new guideline. Can you clarify further, in your opinion, who do you think should be responsible for propagating breaking changes in this package to downstream packages? The author of the breaking change or the product team in question? |
|
I don't know. |
5 participants
This renames
useEventCallbacktouseStableCallback, anduseLatestReftouseValueAsRefto better signify what they're intended to be used for. Added JSDoc to explain each hook better.