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

Sidenav - Convert to TypeScript #2102

Merged
merged 30 commits into from
Jun 12, 2024
Merged

Sidenav - Convert to TypeScript #2102

merged 30 commits into from
Jun 12, 2024

Conversation

didoo
Copy link
Contributor

@didoo didoo commented May 16, 2024

📌 Summary

Converts the SideNav component and subcomponents to TypeScript.

Notice: because of failing tests for ember-lts-3.28 it's not possible at the moment to upgrade ember-stargate package to 0.5.0, so we had to add loose typings for Portal/PortalTarget.

🔗 External links

Jira ticket: https://hashicorp.atlassian.net/browse/HDS-2711


👀 Component checklist

💬 Please consider using conventional comments when reviewing this PR.

@didoo didoo requested a review from a team May 16, 2024 13:53
Copy link

vercel bot commented May 16, 2024

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

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Jun 4, 2024 6:43pm
hds-website ✅ Ready (Inspect) Visit Preview Jun 4, 2024 6:43pm

@didoo didoo marked this pull request as draft May 16, 2024 13:53
@hashibot-hds hashibot-hds added packages/components docs-website Content updates to the documentation website labels May 16, 2024
@didoo
Copy link
Contributor Author

didoo commented Jun 4, 2024

@didoo I had another look at this PR to understand the remaining bits and pushed two commits. SideNav::Base is a very generic component, so I don't see a way around it other than deferring type checking to runtime (so, defining the named blocks as any). If you find this acceptable I can do an overall review.

@alex-ju I've added a few comments, to continue the conversation. question: what do you think if we ask (again) if someone has different/better ideas in the ##proj-hds-ts-migration and/or #tech-modern-ember channels?

update: seems to be failing on ember-lts-3.28, maybe a rebase will help 🤞 looking at the error it may have to do with the ember-stargate bump, although it was meant to work with 3.28

we're using https://github.com/simonihmig/ember-stargate/releases/tag/v0.5.0, which yes seems it should support 3.28, but maybe that's not the case (and I just noticed they released v0.6.0, which drops support for Ember < 4.12!)

what do you think we should do? @meirish is back from PTO next week, should we wait? or try to debug and in case open an issue in the ember-stargate repo?

(I've also rebased, just in case)

@alex-ju
Copy link
Member

alex-ju commented Jun 4, 2024

@didoo I had another look at this PR to understand the remaining bits and pushed two commits. SideNav::Base is a very generic component, so I don't see a way around it other than deferring type checking to runtime (so, defining the named blocks as any). If you find this acceptable I can do an overall review.

@alex-ju I've added a few comments, to continue the conversation. question: what do you think if we ask (again) if someone has different/better ideas in the ##proj-hds-ts-migration and/or #tech-modern-ember channels?

I'm totally up for us to ask around – I'm sure there are ways to make the Types stricter, but wanted to have a fallback to make sure we're not blocked.

@didoo
Copy link
Contributor Author

didoo commented Jun 4, 2024

wanted to have a fallback to make sure we're not blocked.

@alex-ju see my last two commits, let's see if they unblock the tests, and then we decide how to proceed

}
];
};
Element: HdsSideNavBaseSignature['Element'];
Copy link
Contributor

@MelSumner MelSumner Jun 6, 2024

Choose a reason for hiding this comment

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

question: is there documentation somewhere that demonstrates how/why/when to do this kind of element definition specifically? I'm asking because I thought that this was constrained to the existing/valid HTML Element list, so I'm curious to learn more.

followup: thinking through this some more, this is saying "Element has already been defined/given a value, so use that already defined value here" which makes sense in TS land.

Copy link
Contributor Author

@didoo didoo Jun 6, 2024

Choose a reason for hiding this comment

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

Answering on the base of my limited knowledge/experience with TS: this is the way in which one "propagates" a known type across components/subcomponents: if component "A" is "consuming" a prop xyz (let's say of type string) from a component "B", in the signature of "A" we don't use string, but a reference to the "B" prop (B[xyz]). In this way, if in the future one needs to update xyz in "B" to be string | number, they don't have to remember to update manually the type also in the components (in our example "A") that consume the component "B", but it will be done automatically, because is already a reference to B[xyz]. Makes sense?
This Element: HdsSideNavBaseSignature['Element']; works exactly in this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alex-ju can you keep me honest on the above? 👆

Copy link
Member

Choose a reason for hiding this comment

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

what Cristiano said is accurate; to help cascading/propagating types we oftentimes use the signature of the referenced component

Copy link
Contributor

@MelSumner MelSumner left a comment

Choose a reason for hiding this comment

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

I don't think I see anything amiss. I agree we should figure out the stargate thing but we could also save that for after we're able to upgrade, maybe.

@didoo didoo merged commit fb934c6 into main Jun 12, 2024
16 checks passed
@didoo didoo deleted the sidenav-ts-conversion branch June 12, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-website Content updates to the documentation website packages/components typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants