Skip to content

Conversation

amcclain
Copy link
Member

…form typing

Hoist P/R Checklist

Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.

  • Caught up with develop branch as of last change.
  • Added CHANGELOG entry, or determined not required.
  • Reviewed for breaking changes, added breaking-change label + CHANGELOG if so.
  • Updated doc comments / prop-types, or determined not required.
  • Reviewed and tested on Mobile, or determined not required.
  • Created Toolbox branch / PR, or determined not required.

If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.

Pull request reviewers: when merging this P/R, please consider using a squash commit to
collapse multiple intermediate commits into a single commit representing the overall feature
change. This helps keep the commit log clean and easy to scan across releases. PRs containing a
single commit should be rebased when possible.

@ghsolomon
Copy link
Contributor

This definitely seems like an improvement and agree that the cross-platform type imports should be erased. I think I would prefer a shared ColChooserModel interface that the 2 platform-specific classes implement over the union type in this case for a couple reasons:

  • With a single shared interface, there would be one place for developers to look as opposed to the union type which requires looking at the 2 implementations to determine what is shared.
  • Along similar lines, for our own development, the single shared interface gives us a clean contract across both of our platform-specific implementations as opposed to the other way around.

It would add complexity, but would we want this to be generic? I can't really think of a way to infer the desktop vs mobile type -- would likely need to be specified explicitly, so that might not really be any better than a cast.

@amcclain amcclain changed the title Use import type for improved GridModel.colChooserModel cross-plat… Add IColChooserModel cross-platform interface Aug 1, 2025
@amcclain
Copy link
Member Author

amcclain commented Aug 1, 2025

@ghsolomon updated - WDYT? Improvement?

@ghsolomon
Copy link
Contributor

@ghsolomon updated - WDYT? Improvement?

I like this better. Should IColChooserModel extend HoistModel in case someone wanted to access members of HoistModel?

As far as naming conventions, have we used the "I" prefix before? I understand it here to avoid colliding with the concrete implementations, but from what I gather the general Typescript community tends to discourage it as opposed to in Java, C#, etc. Here's Microsoft's internal Typescript coding guidelines, which they emphasize only apply to TS contributors, but still could be worth noting: https://github.com/microsoft/TypeScript/wiki/Coding-guidelines. See (2) under Names (Do not use I as a prefix for interface names.)

That said, I'm not sure what else we'd call this? BaseColChooserModel? CrossPlatformColChooserModel? Maybe in this case best to stick with the prefix, but wanted to throw this out.

@amcclain
Copy link
Member Author

amcclain commented Aug 13, 2025

That said, I'm not sure what else we'd call this? BaseColChooserModel? CrossPlatformColChooserModel? Maybe in this case best to stick with the prefix, but wanted to throw this out.

Yes, I also am not a fan of the I prefix, but this was exactly my dilemma. I figured if we were ever going to use it, perhaps these cross platform interfaces would be the place.

In hindsight, perhaps we should have gone with [Desktop|Mobile]ColChooserModel for the implementations... That would also have reduced likelihood of grabbing wrong import / IDE confusion. Then the interface simply ColChooserModel. Too late to consider that change?

@lbwexler not urgent but interested in your thoughts here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants