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

HDS-2717: Toast - Convert to TypeScript #2023

Merged
merged 5 commits into from
Apr 9, 2024
Merged

Conversation

WenInCode
Copy link
Contributor

@WenInCode WenInCode commented Mar 26, 2024

πŸ“Œ Summary

Converts Hds::Toast to Typescript

πŸ› οΈ Detailed description

  • Converts Hds::Toast to Typescript
    • I initially tried the recommended approach of using templateOnlyComponent to type the Toast component as it had no backing class. However, I was getting Typescript errors when trying to pass it the type signature stating that templateOnlyComponent does not take type arguments. It seems like that only became available in later version of Ember. I thought it would be fine given this is an embroider addon but idk, typescript was not happy! Willing to revisit this if anyone has any ideas.
  • Updates the Hds::Alert onDismiss argument type to take the MouseEvent and then any number of args with any value. I originally had this set to () => void, which works great if you never want to handle arguments on the action handler, but it falls on it's face if you do. I thought it would be better to switch to always handle an event, and then any number of arguments of any value, as you can pass in whatever you would like with a (fn) helper. I used any here because you can't type the handler functions additional args if you set it to unknown

πŸ“Έ Screenshots

Screenshot 2024-03-26 at 2 59 49β€―AM Screenshot 2024-03-26 at 3 05 11β€―AM Screenshot 2024-03-26 at 3 05 19β€―AM

πŸ”— External links

Jira ticket: HDS-2717


πŸ‘€ Component checklist

πŸ’¬ Please consider using conventional comments when reviewing this PR.

Copy link

vercel bot commented Mar 26, 2024

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Updated (UTC)
hds-showcase βœ… Ready (Inspect) Visit Preview Apr 4, 2024 5:02pm
hds-website βœ… Ready (Inspect) Visit Preview Apr 4, 2024 5:02pm

@WenInCode WenInCode marked this pull request as ready for review March 26, 2024 09:16
@Dhaulagiri
Copy link
Contributor

It seems like that only became available in later version of Ember

Like if the components package was on a later version of Ember you would not have this problem? Or something else?

@aklkv
Copy link
Contributor

aklkv commented Mar 26, 2024

@WenInCode I am curious ti dig into this issue, did this fail?

@aklkv
Copy link
Contributor

aklkv commented Mar 26, 2024

Also side note, I found this codemod very useful in adding signatures it takes care of these conversions. you can supply the exact component path to be converted in order to avoid converting other components

Comment on lines +3 to +5
export interface HdsToastSignature extends Omit<HdsAlertSignature, 'Args'> {
Args: Omit<HdsAlertSignature['Args'], 'type'>;
}
Copy link
Member

Choose a reason for hiding this comment

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

This is a good example of how TypeScript could help us spot issues. While this interface is accurate (the Toast component doesn't expose a @type argument) we didn't document it as such in the component API. I will raise a follow-up PR to correct that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I initially had it implementing the Alert interface and realized afterwards that inline is hardcoded in Toast. πŸ‘

Comment on lines 1 to 4
import Component from '@glimmer/component';
import type { HdsToastSignature } from './types.ts';

export default class HdsToastComponent extends Component<HdsToastSignature> {}
Copy link
Member

@alex-ju alex-ju Apr 4, 2024

Choose a reason for hiding this comment

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

Having a backing class is a good fallback, but considering we expect to have many other components as templates only, I'd try to get it right now so we can replicate this in the future.

You probably tried something like this and it complained with something along the lines of 'Expected 0 type arguments, but got 1.'. This, as you mentioned in the PR description, means it picks up @types/ember__component with a 3.x version (which seems to be required by a 3rd degree dependency in website, so unrelated to this package).

Suggested change
import Component from '@glimmer/component';
import type { HdsToastSignature } from './types.ts';
export default class HdsToastComponent extends Component<HdsToastSignature> {}
import TemplateOnlyComponent from '@ember/component/template-only';
import type { HdsToastSignature } from './types.ts';
const HdsToastComponent = TemplateOnlyComponent<HdsToastSignature>();
export default HdsToastComponent;

Instead of @types/ember__component/template-only.d.ts, it should be using ember-source/types/stable/@ember/component/template-only.d.ts (available in our ember version), however that's not happening and it's puzzling πŸ€”

Copy link
Member

Choose a reason for hiding this comment

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

the only way around it is to be more explicit about which path to use to resolve TemplateOnlyComponent – I pushed a commit to illustrate that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah that's exactly what it was. Appreciate the commit. I tried it out and it LGTM

For some reason TypeScript picks up `@types/ember__component/template-only.d.ts`, instead of `ember-source/types/stable/@ember/component/template-only.d.ts` so we're a bit more explicit about which path it should use
@WenInCode
Copy link
Contributor Author

@alex-ju thanks for helping with the resolution issue. The changes LGTM I think this is ready for another set of πŸ‘€.

@WenInCode WenInCode merged commit f2a8b91 into main Apr 9, 2024
16 checks passed
@WenInCode WenInCode deleted the HDS-2717/convert-toast-ts branch April 9, 2024 16:24
@hashibot-hds hashibot-hds mentioned this pull request Apr 9, 2024
@alex-ju alex-ju changed the title HDS-2717: Hds::Toast - Converts to Typescript HDS-2717: Toast - Convert to TypeScript May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants