ui-utils: Add new package with two type checking functions #28028
Conversation
|
Size Change: +4.29 kB (0%) Total Size: 1.3 MB
ℹ️ View Unchanged
|
|
@saramarcondes Thank you for working on this 🙏 ! I checked out the failing tests. The Static Analysis one is reporting: I'm not sure if your change is related. I'm seeing common E2E Admin 4 failures across recent commits: |
|
The My initial PR attempt added 🤗 Update: Linking to my original PR, just in case it helps: |
There was a problem hiding this comment.
@wordpress/compose contains higher-order components (now considered legacy) and React hooks to reuse with React components. I don't think that those utils are good fit here.
In general, all those utils are so simple or re-exported from lodash that we could as well inline them as we need them. Many of them can be also replace with a simple check like:
x === null;
y typeof Number;
z === undefined;In general, we aim to stop using lodash at scale as discussed in #17025. It's quite a challenge to refactor old code but if it could be avoided for new code then it would be fantastic.
I agree, I just wasn't sure where to put it 😅 However it sounds like the preferrence is not to add this at all, in the first place. Personally I think that's fine, opting for hand written type checks accomplishes much of the same thing as these functions. There are, however, a couple that are particularly useful for G2. |
If it isn't possible to keep them internal to The reality of having another package is a bit different in the WordPress world than in web apps that have a single entry point and use code splitting. Here we will create another entry point that is also exposed as |
Unfortunately they're used in multiple different packages like I'll update this PR with the pared down version. |
|
I think that @youknowriad found some way to keep packages public on npm and hide them from exposing in WordPress globals. He would be the best person to recommend how to move forward. |
sarayourfriend
changed the title
is util
|
@gziolo None of the G2 packages should probably be exposed in the global object at all (at least not for a long time, if not never) so if we can figure that out for this package it would be a boon for future packages like |
There was a problem hiding this comment.
The new utils and package are a 👍 from me! Thank you so much @saramarcondes !!
As @gziolo has mentioned, hopefully we'll be able hide them from the global WP object 🙏
| @@ -1817,6 +1817,12 @@ | |||
| "markdown_source": "../packages/token-list/README.md", | |||
| "parent": "packages" | |||
| }, | |||
| { | |||
| "title": "@wordpress/ui-utils", | |||
There was a problem hiding this comment.
ui-utils as a name doesn't make sense for me. These utilities can be useful for any code. Why not @wordpress/is
There was a problem hiding this comment.
I guess this comment doesn't make sense if we decide to drop the package.
There was a problem hiding this comment.
ui-utils as a name doesn't make sense for me.
The reason for this PR and for this package is to provide a space for us to migrate the other utilities found in the G2 Component system:
https://github.com/ItsJonQ/g2/tree/master/packages/utils/src
Why not @wordpress/is
I'm happy renaming it to @wordpress/utils package if that were the case.
There was a problem hiding this comment.
We used to have a @wordpress/utils a long time ago and we removed it in favor of smaller packages with clear purpose. A @wordpress/is would have been a good one here but IMO, lodash offers already all of these functions and since lodash is already used across our codebase, I'd just continue using it instead of replacing it with our custom lodash.
| * @param {T | undefined | null} value The value to check. | ||
| * @return {value is T} Whether the value is undefined. | ||
| */ | ||
| const isDefined = ( value ) => value !== null && value !== undefined; |
There was a problem hiding this comment.
This already exists in lodash isNil
There was a problem hiding this comment.
You're right, but if we'd like to remove lodash at scale, as described by #17025, we'd need to replace this utility (it's a quite useful one, if you search through G2 for is.defined you'll see it's used almost everywhere).
We could replace it with ! isNil but that doesn't accomplish getting rid of lodash at all.
There was a problem hiding this comment.
I'm not sure I agree with "getting rid of lodash by adding our own utils". I think getting rid of lodash by using native functions is definitely a good thing but other than that:
- Lodash can be replaced with lodash-es for npm consumers
- Lodash is only loaded once in WordPress, it's better that bundling the same utils multiple times.
There was a problem hiding this comment.
Sounds good to me. We'll replace is.defined with ! isNil everywhere then?
| * @param {TemplateStringsArray | import('create-emotion').Interpolation} value The value to check. | ||
| * @return {value is import('create-emotion').ObjectInterpolation} Whether the value is an ObjectInterpolation. | ||
| */ | ||
| const isObjectInterpolation = ( value ) => isPlainObject( value ); |
There was a problem hiding this comment.
why not just use isPlainObject directly?
There was a problem hiding this comment.
The usefulness of this function comes primarily from the type defintions. It's impossible to refine the type any other way other than through type definitions which TypeScript will trust implicitly. This allows the newly added packages to be fully and completely typed.
For example, given the following (real) code example:
/**
* @param {TemplateStringsArray | import('create-emotion').Interpolation<undefined>} template
* @param {import('create-emotion').Interpolation<undefined>[]} args The styles to compile.
* @returns {ReturnType<compile>} The compiled styles.
*/
export function css(template, ...args) {
if (isObjectInterpolation(template)) {
return compile(responsive(template));
}
if (Array.isArray(template)) { // => TemplateStringsArray
for (let i = 0, len = template.length; i < len; i++) {
const n = template[i];
if (isObjectInterpolation(n)) {
template[i] = responsive(n);
}
}
return compile(template, ...args);
}
return compile(template, ...args);
}If you replace isObjectInterpolation with isPlainObject and responsive's first argument will not be refined enough to be accepted as responsive only accepts an ObjectInterpolation for it's first argument.
There was a problem hiding this comment.
I don't think it's a valid reason for us at least to add a new package to be honest. Can't this be solved by an explicit type cast instead?
There was a problem hiding this comment.
Well, this package isn't complete as it stands, so it's justification can't live on just these two utilities. The issue linked in the PR description has a short list of utilities that would live in this package (and those are just the ones used by the Text and Truncate components, there are many more if you check out the G2 repo).
FWIW I hadn't tried to type cast it but it does work, so we can remove this utility if you'd prefer to just use type casts (that's all this function does anyhow).
Regardless, I'll open a separate PR with a different utility adding the same ui-utils package for getOptimalTextShade, getDisplayName, and getComputedColor as the issue linked in the PR describes. This was just meant as an incremental way of doing it without moving more code at one time than is possible for a person to effectively review.
There was a problem hiding this comment.
I think adding packages shouldn't be seen as something we should do easily especially in the context of WordPress, so I'd personally feel better if we start by doing inline utilities in the "components" package and discuss whether it deserves a separate package or not as the usage grows.
There was a problem hiding this comment.
What about utilities used by mulitple packages like getDisplayName? Where should that live? We can't introduce them in components as that would create a circular dependency. We can duplicate the code in the packages if you'd prefer that. Or we can export them from an unrelated package, but then that package will be responsible for something that it shouldn't care about.
There was a problem hiding this comment.
What does getDisplayName do?
There was a problem hiding this comment.
/**
* Gets the displayName of a React Component or element.
*
* @param {string | import('react').ComponentType} tagName
*
* @return {string} The display name of the Component / tagName.
*/
export function getDisplayName(tagName) {
const displayName =
typeof tagName === 'string'
? tagName
: tagName.displayName || tagName.name || 'Component';
return displayName;
}There was a problem hiding this comment.
So looking a g2. It seems to be only used once. That said it seems to be used to compute components display name? In fact we have something close to it in createHigherOrderComponent in @wordpress/compose so it does seem like a natural place for it if its usage grows.
There was a problem hiding this comment.
Ah, you're right, I thought context used it as well but I was wrong. We'll inline that one too I guess.
Description
This is part of #28020. It adds a new
ui-utilspackage which will contain the various utilities useful for building user interfaces like the two functions included in this PR.How has this been tested?
Build passes (these utilities are currently unused)
Types of changes
New feature/package.
Checklist: