Components utils: use Object.assign instead of { ...spread } syntax
#39932
Conversation
This file contains 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
…o avoid errors in the code generated by TypeScript
ciampo
added
[Type] Bug
|
Size Change: +77 B (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
ciampo
commented
Mar 31, 2022
Comment on lines
+7
to
+17
| - Update `BorderControl` and `BorderBoxControl` to allow the passing of custom class names to popovers ([#39753](https://github.com/WordPress/gutenberg/pull/39753)). | ||
|
|
||
| ### Internal | ||
|
|
||
| - `BaseControl`: Convert to TypeScript ([#39468](https://github.com/WordPress/gutenberg/pull/39468)). | ||
|
|
||
| ### New Features | ||
|
|
||
| - Add `BorderControl` component ([#37769](https://github.com/WordPress/gutenberg/pull/37769)). | ||
| - Add `BorderBoxControl` component ([#38876](https://github.com/WordPress/gutenberg/pull/38876)). | ||
| - Add `BorderControl` component ([#37769](https://github.com/WordPress/gutenberg/pull/37769)). | ||
| - Add `BorderBoxControl` component ([#38876](https://github.com/WordPress/gutenberg/pull/38876)). | ||
|
|
There was a problem hiding this comment.
Changes on these lines are only about aligning the white spacing at the beginning of every list item
ntsekouras
approved these changes
Mar 31, 2022
mirka
approved these changes
Mar 31, 2022
There was a problem hiding this comment.
Really interesting 🤔 After reading up on it, I do see how the internal implementation could be different between spreads and assigns. I can't seem to get a minimal repro so I wonder if there's something else involved too. But that's just for curiosity.
Co-authored-by: Nik Tsekouras <ntsekouras@outlook.com>
|
I also personally don't really know why this was happening — it definitely looks it's a TypeScript bug, but I also found it difficult to reproduce it in isolation. Somehow when using the spread syntax in the affected, TypeScript doesn't seem to be able to fully "expand" the object that is being spread. In the case of I also found some differences between |
|
I wonder if this is something an option in |
That is also a possibility, although I haven't had the time to look into it after finding a fix. I'll put it in my list of tasks, but anyone feel free to also explore it! |
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.
What?
The changes in this PR refactor the code in the
utilsfolder of the@wordpress/componentspackage, switching toObject.assign()instead of using the{ ...spread }syntax.Why?
While working on #39436, I discovered the following issue, which can be replicated by running
npm run build:packagesand inspecting the following generated files:packages/components/build-types/utils/config-values.d.ts: this file actually contains a couple of circular dependency errors:See screenshot
packages/components/build-types/utils/colors-values.d.ts: in this file, we can observe that effectively the type of theCOLORSobject doesn't include thewhitekey.How?
In both cases, I was able to solve the issue by using
Object.assigninstead of the{ ...spread }syntax.Testing Instructions
On trunk:
npm run distclean && npm ci && npm run build:packagespackages/components/build-types/utils/config-values.d.tsand notice the TypeScript errors.packages/components/build-types/utils/colors-values.d.ts, and notice how the type of theCOLORSobject doesn't include thewhitekey.Apply the changes from this PR:
npm run distclean && npm ci && npm run build:packagespackages/components/build-types/utils/config-values.d.ts, there shouldn't be any TypeScript errorspackages/components/build-types/utils/colors-values.d.ts, the type of theCOLORSobject should include thewhitekey.