fix: land the mergeRefs, buildContext and mergeProps fixes from #277, #279 and #281 - #486
Merged
Merged
Conversation
Co-authored-by: wo-o29 <woogur29@gmail.com>
…ample Co-authored-by: wo-o29 <woogur29@gmail.com>
Co-authored-by: wo-o29 <woogur29@gmail.com>
🦋 Changeset detectedLatest commit: 921369f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
Size Change: +347 B (+0.32%) Total Size: 109 kB 📦 View Changed
ℹ️ View Unchanged
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #486 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 69 69
Lines 2261 2280 +19
Branches 728 737 +9
=========================================
+ Hits 2261 2280 +19 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Lands the three community PRs from @wo-o29 (#277, #279, #281) on the current package layout, one commit each with the original author as co-author. The problems they report are real; two of them are applied in a slightly different shape, noted below.
mergeRefs— forward React 19 ref cleanups (#281)React 19 lets a callback ref return a cleanup function.
mergeRefscalled the inner refs and discarded their return values, so React never saw a cleanup, called the merged ref withnullinstead, and the inner cleanup never ran.The merged ref now collects what each inner ref returns. If at least one returned a cleanup, the merged ref returns a cleanup that runs those and resets the remaining refs to
null. If none did, it returns nothing, exactly as before, so React 18 keeps itsnullcall and does not log its "callback ref should not return a function" warning. This is the approach Radix'scomposeRefsuses; #281 always returned a cleanup, which would trigger that warning on React 18.Tests render a real element and assert the cleanup runs on unmount, that mixed refs (cleanup, object ref,
null, plain callback) are all handled, and that nothing is returned when no ref asks for a cleanup.buildContext—displayName, docs example, type mismatch (#277)Provider.displayNameis now`${contextName}Provider`so DevTools shows which context a provider belongs to.nullas the default values, which the signature rejects. It now passes an object.ContextValuesType | undefinedbut the provider suppliednullbehind anascast. The provider now suppliesundefined, which removes the cast. refactor(buildContext): added displayName and fixed useInnerContext branch handling #277 unified onnullinstead;undefinedkeeps the existing type and the!= nullguards, so a JS caller still passingnullfrom the old docs keeps getting the "must be used within" error.mergeProps— remove the dead branch (#279)pushPropalready skipsundefinedvalues at the top of the loop, so the innerelse if (curr[key] !== undefined)was always true. Folded intomergeFunction(...) ?? curr[key]. ThemergeStylesimplification from #279 is not included: droppingif (a == null) return bchanges the identity of the returned style object.Checklist
yarn run fixto format and lint the code and docs?yarn run test:coverageto make sure there is no uncovered line?