Fix type errors for Button's use of Icon data.

[willnationsdev]

No description provided.

[changeset-bot]

🦋 Changeset detected

Latest commit: 84ea64b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
svelte-ux	Patch

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

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
svelte-ux	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 12, 2023 3:44am

[what-the-diff]

PR Summary

• Introduced a New Component: .changeset/slimy-starfishes-report.md
  • This change deals with resolving certain type errors associated with how the Button component uses data from the Icon component.
• Modifications to: packages/svelte-ux/src/lib/components/Button.svelte
  • Introduced a new variable iconData and a new function isIconComponentProps, aiming to ensure better data handling.
  • Improved the Button component's interaction with the Icon component by updating how it uses iconData.
• Updates to: packages/svelte-ux/src/lib/components/Icon.svelte
  • Enhanced the stability of the component by adding a check for svgUrl's existence before deleting it from the cache. This helps prevent unforeseen errors due to null values.

[willnationsdev]

Just found something to improve with this, so don't merge it quite yet.

[willnationsdev]

Okay, now the PR introduces inclusive input types for supplying icon data as arguments for components whether it be a null, undefined, string, IconDefinition, or an object with all the properties of an Icon component (which should include Icon components themselves). Then it extracts the data representation of the input and passes that along to the inner Icon component's data argument. It can therefore accommodate a variety of input types and empower ancestor components to seamlessly pass those arguments to descendants until they finally reach an Icon, and without introducing varying requirements on the data types of the passed data along the way. Should be able to gradually update component arguments to use these new IconInput types if that sounds good to you.

[willnationsdev]

Amended the changeset to provide more information due to the above changes.

[techniq]

This looks great, and a huge win. Just a few minor notes

[techniq]

There is a isLiteralObject() util that could help refine this a little.

I'm also not sure typeof(v['classes']) === "object" && "path" in v is the correct check.

I wonder if asIconData() should just be...

export function asIconData(v: IconInput): IconData {
  return ('data' in v) ? v.data : v;
}

I might be overlooking something though.

[willnationsdev]

There is a isLiteralObject() util that could help refine this a little.

I agree, that's better to use.

I'm also not sure typeof(v['classes']) === "object" && "path" in v is the correct check.

When I was writing this initially, I had thought that ComponentProps<Icon> required an object with all the same properties of Icon, but I can see now that it actually is like a Partial wrapper around that, so yeah, this definitely would NOT be the right check. Good catch.

Since IconDefinition has a more consistent structure & because it's the only other supported "object" type, we should just check to see if the given v object does NOT have certain properties that all IconDefinition objects are required to have and that Icon lacks. Therefore, a single property check can, by process of elimination, validate which type we're dealing with.

I wonder if asIconData() should just be...

In theory, you'd be right (if it were just JS), but in order for TypeScript to rule out the possibility that the IconInput parameter is actually a ComponentProps<Icon>, you either have to exhaustively check all possible properties (far more work than is actually necessary) or write a function that forcibly coerces the type understanding via a type guard return value (<param> is <type> return type).

[techniq]

fwiw, there is a typeGuards utils with hasProperty(), etc. Might not be useful here

[techniq]

<Icon> used to just accept <Icon path="...">

...then it was extended to allow <Icon svg="...">, ```

...then it was extended to have a simple <Icon data="..."> prop and infer which of the props was intended (path, svg, svgUrl), so I call that a feature to allow TextField to take in more options than just path :).

svgUrl is quite useful when your loading over the network, and not populating your bundle with all potential options.

[willnationsdev]

Hehe, I wasn't saying that it doesn't support different kinds of inputs. More that, we have instances of wrappers with inner wrappers with inner-inner Icons, etc. and the allowed data types for those wrapper components' distinct arguments do not always match, leading to inconsistency that has the potential to force type errors for no reason. I actually have to compliment this project for the diversity of data formats that Icon supports.

[techniq]

:). It's been battled tested in a lot of use cases. One thing I struggle with is getting the time to show all the niceties in examples :). <Table> is under represented for how much it packs ;)

[techniq]

What if we call this getIconData()?

[willnationsdev]

I considered that initially, and I'm good with that if you prefer it. Only reason I chose as was because get sounded like it was consistently extracting data, but just in potentially different/abstracted ways whereas as communicates that the object in question might already be the return type (IconData) and conveys that the function's purpose is ultimately type conversion rather than extraction. Up to you.

[techniq]

We can leave it as is (pun intended) :)