Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add glint types #1861

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from
Draft

Conversation

vlascik
Copy link

@vlascik vlascik commented Jan 25, 2023

First stab at glint types for e-bootstrap. Couple of notes:

  1. Didn't bother with typing helpers, they are mostly not interesting for consumers of the addon anyway.
  2. Types are definitely wrong at places. Docs app passes (except for helpers), my app passes, but that doesn't cover all of the addon and its components. Some components are fairly unclear and could use proper Octane migration first.
  3. I mostly didn't bother with WithBoundArgs for yielded components.
  4. It's quite hard to guess the intentions of the components sometimes, so some of the types are just wild guesses.
  5. There are both .ts and .d.ts files here, .ts are mostly needed for template only components. That means that there probably should be a pre-publish step, like ember ts:precompile, but that produces files in /components folder, not sure if that's a good idea.

All in all, this PR so far should take it some 80-90% there, but there will be some more work needed. However after doing this much menial typing, I'm kind of spent, so someone else will probably need to invest couple of minutes (or hours, no idea) into checking and fixing these too.

Anyway, hope these'll be useful.

Copy link
Contributor

@jelhan jelhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for starting to work on this! This is huge!

I didn't had the time to review everything. But I though it might be helpful to submit my first findings anyways to get the discussion rolling. I fear there is at least one bigger question not answered yet: How to deal with types, which depend on the Bootstrap SASS theme being used? Please find more details in one of my review comments about that one.

addon/components/bs-button.d.ts Outdated Show resolved Hide resolved
interface BsFormElementSignature<T> {
Element: HTMLDivElement;
Args: {
controlType?: 'text' | 'checkbox' | 'radio' | 'switch' | 'textarea' | string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be awesome if we could detect the registered control components. Not sure how that would work. But I guess it would be only needed as interim solution anyways. I assume we are going to deprecate that feature when first-class component templates are available.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as mentioned elsewhere, this should be just an arg for <input type={{controlType}} html attribute, so string should be enough.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interface BsFormElementSignature<T> {
Element: HTMLDivElement;
Args: {
controlType?: 'text' | 'checkbox' | 'radio' | 'switch' | 'textarea' | string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think / hope that passing a component is supported as well. But have never tested to be honest.

Suggested change
controlType?: 'text' | 'checkbox' | 'radio' | 'switch' | 'textarea' | string;
controlType?: 'text' | 'checkbox' | 'radio' | 'switch' | 'textarea' | string | typeof Component<unknown>;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is propably not the place to pass a component type, but rather <input type={{controlType}} input type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ember Bootstrap supports custom control types. E.g. the ember-bootstrap-power-select addon provides an integration with Ember Power Select allowing it to be used as a control type.

<BsForm as |form|>
  <form.element @controlType="power-select" />
</BsForm>

Current that relies on naming convention. If being passed a power-select control type, Ember Bootstrap tries to resolve a component <BsForm::Element::Control::PowerSelect>. If that does not exists it falls back to a default control type.

That lookup mechanism is not fitting well with Embroider and tree-shaking. With first class component templates passing the component directly would feel much more comfortable:

import PowerSelectControl from 'ember-bootstrap-power-select';

<template>
  <BsForm as |form|>
    <form.element @controlType={{PowerSelectControl}} />
  </BsForm>
</template>

I think / hope that is already supported today. But have never double checked.

addon/components/bs-form/element.d.ts Outdated Show resolved Hide resolved
addon/components/bs-form/element.d.ts Outdated Show resolved Hide resolved
addon/components/bs-form.d.ts Outdated Show resolved Hide resolved
addon/components/bs-button.d.ts Outdated Show resolved Hide resolved
addon/components/bs-button.d.ts Outdated Show resolved Hide resolved
addon/components/bs-button.d.ts Outdated Show resolved Hide resolved
@vlascik
Copy link
Author

vlascik commented Feb 5, 2023

@jelhan thanks for the comments, but I think the best way, instead of discussing these types going back and forth for possibly a really long time, would be just you or other maintainers with a better knowledge of the addon taking this PR as a starting point and running with it.

I got it as far as I could, most of the grind should be done, but these definitely need refining past what I can do. I don't think I have much to add from this point on (don't know the addon that much), so doing it this way may be the fastest way to get it through the finish line.

@jelhan
Copy link
Contributor

jelhan commented Feb 6, 2023

@jelhan thanks for the comments, but I think the best way, instead of discussing these types going back and forth for possibly a really long time, would be just you or other maintainers with a better knowledge of the addon taking this PR as a starting point and running with it.

Could do so. But I fear neither @simonihmig nor me having much time currently to work on it. So that may delay shipping it quite a bit.

@vlascik
Copy link
Author

vlascik commented Feb 6, 2023

Could do so. But I fear neither @simonihmig nor me having much time currently to work on it. So that may delay shipping it quite a bit.

I think I have another way to check these types in my app, so I will try to fix them a bit more, but ultimately, other people will have to check them too, my app doesn't use every component or feature of this addon by far.

I wouldn't personally be too concerned about these types being wrong at places though - they don't affect the runtime, so worst case I would put a warning at the types - and let people who get the typing squiggles fix them over time on their own.

@vlascik vlascik mentioned this pull request Feb 7, 2024
32 tasks
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.

None yet

2 participants