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

feat(chore): added types definitions #2822

Merged
merged 4 commits into from
Jun 19, 2023
Merged

Conversation

prudho
Copy link
Contributor

@prudho prudho commented Jun 15, 2023

Long time overdue, sorry about that !

Description

This PR adds type definitions for Fomantic's components, enabling them to be more usable on code editors. Typings are now integrated into the actual repository, rather than in the DefinitelyTyped repository, allowing us a better control over type updates and quick fixes. They've been updated to the actual 2.9.2 release, so they should be pretty accurate.

To use them, there's nothing to do since they're included in the package.json. Users should also include jquery types as well in their package.json: npm install --save-dev @types/jquery

There's maybe some comments to add, but it can be released now.

@fomantic/maintainers should I also create a pull request at https://github.com/fomantic/Fomantic-UI-CSS ?

Screenshot

image

image

Closes

#396, #1205, #2742

@prudho prudho added type/feat Any feature requests or improvements lang/javascript Anything involving JavaScript type/discussion Anything which is up for discussion type/build Anything related to the build process state/awaiting-reviews Pull requests which are waiting for reviews labels Jun 15, 2023
@prudho prudho added this to the 2.9.3 milestone Jun 15, 2023
@prudho prudho requested a review from a team June 15, 2023 14:43
@lubber-de
Copy link
Member

lubber-de commented Jun 15, 2023

Cool and thanks a lot for doing this 👍🏼 I believe you make many people happy 😃

What should we do with https://github.com/fomantic/DefinitelyTyped/tree/fomantic-ui then?
Delete the repo?

Regarding Fomantic-UI-CSS. We are always creating that out of a new FUI release, so usually dont maintain separate PRs there.
I think, for future releases we only need to add the "types" folder and add the "types" key inside package.json just like in this PR, right?

Btw https://github.com/fomantic/Fomantic-UI-LESS also has the single js files for the modules, so should we add the types there as well?

Copy link
Member

@lubber-de lubber-de left a comment

Choose a reason for hiding this comment

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

LGTM

@prudho
Copy link
Contributor Author

prudho commented Jun 16, 2023

The DefinitelyTyped repo can be deleted, right.

For the Fomantic-UI-CSS and Fomantic-UI-LESS repo, yes, we just need to add the types folder and add the line in package.json.

@mvorisek
Copy link
Contributor

mvorisek commented Jun 16, 2023

All JS files should be tested againts the TS interfaces and any missing/extra method/property should fail CI.

@lubber-de
Copy link
Member

All JS files should be tested againts the TS interfaces and any missing/extra method/property should fail CI.

That would be worth another PR as it does not seem to be easy. The whole special "behavior" concept (string as first parameter of module call magically matches existing (nested) javascript method tree) would basically work for each existing method inside the modules, but only the "intended" methods are listed in the docs (and only those were covered by prudho here)

A CI, as you suggested, would complain about a lot of unfetched methods as those are not intended to be mentioned.
Of course, it would be consistent to document all of them, even if possibly just used internally by the module, but this would also need a ton of missing documentation for each function. As always, volunteers welcome 😉

I believe this only works out nicely when recoding each module from scratch with a different code structure concept and modern techniques where types and docs can be created/fetched/generated automatically out of the source without the need to manually touch them.

So, I'll merge this PR as it is now and will look for possibly missing new settings in 2.9.3 as this PR "only" covers 2.9.2 which already must have been a hell of work.

@prudho
Copy link
Contributor Author

prudho commented Jun 19, 2023

@mvorisek yes, this is something i'd like to implement too. A real testing system for each function/property would be great, but like @lubber-de said, this would be a rather large project to test basically everything.

For now, and since we're using an outdated code base, all this work can be done manually, and trust me, there's A LOT of work to do so.

@lubber-de lubber-de changed the title Add types definitions feat(chore): added types definitions Jun 19, 2023
@lubber-de lubber-de merged commit eabc58e into fomantic:develop Jun 19, 2023
@lubber-de lubber-de removed the state/awaiting-reviews Pull requests which are waiting for reviews label Jun 19, 2023
@lubber-de
Copy link
Member

lubber-de commented Jun 19, 2023

The DefinitelyTyped repo can be deleted, right.

@prudho As discussed, I just deleted the DefinitelyTyped fomantic fork

@lubber-de lubber-de added the type/types Anything related to types label Jun 27, 2023
@prudho prudho deleted the types branch August 2, 2023 14:36
lubber-de pushed a commit that referenced this pull request Dec 19, 2023
This PR improves #2822, the Typescript is now tested if compiles and linted againts our coding style.
lubber-de pushed a commit to lubber-de/Fomantic-UI that referenced this pull request Feb 11, 2024
This PR improves fomantic#2822, the Typescript is now tested if compiles and linted againts our coding style.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/javascript Anything involving JavaScript type/build Anything related to the build process type/discussion Anything which is up for discussion type/feat Any feature requests or improvements type/types Anything related to types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants