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: add type declarations to distro #65

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

MartyDashP
Copy link

@MartyDashP MartyDashP commented Jan 7, 2025

Proposed Changes

Add type declarations to distribution package

Checklist

To ensure you provided everything we need to look at your PR:

  • Brief textual description of the changes present
  • Visual demo attached
  • Steps to try out present, i.e. using the @bpmn-io/sr tool
  • Related issue linked via Closes {LINK_TO_ISSUE} or Related to {LINK_TO_ISSUE}

@CLAassistant
Copy link

CLAassistant commented Jan 7, 2025

CLA assistant check
All committers have signed the CLA.

@MartyDashP
Copy link
Author

I think this improvement will make it easier to work with moddle when developing extensions for bpmn-js.

@MartyDashP MartyDashP force-pushed the described-type-definitions branch from 6646aae to df78b1c Compare January 7, 2025 13:33
@nikku
Copy link
Member

nikku commented Jan 7, 2025

@MartyDashP Thanks for your contribution 🚀

This PR fits well into our ongoing initiatives to offer types for our libraries.


As a matter of fact, exported types are a contract, an we need to ensure that we have tests for them; understand when they break accidentally, but also to show proposed use (example). Could you investigate a way how to add such tests for the generated types?

Some learnings from typing some of our core libraries (diagram-js, bpmn-js):

  • A good pattern is to have a FileName.spec.ts file next to the actual JS file (example), and do a tsc --noEmit run to verify the declarations generated work as intended.
  • To consider: What format do the type definitions have, are they clean? If not then bio-dts is a helpful tool to ensure the resulting type definitions are meaningful, and clean.

@nikku nikku added the enhancement New feature or request label Jan 7, 2025
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

We need test coverage for any types that we generate. Added some thoughts.

@MartyDashP
Copy link
Author

@nikku Thank for the feedback. I'll try to cover all exported types with tests. I can't promise that it will be done quickly, but I will do my best 😀

@MartyDashP MartyDashP marked this pull request as draft January 8, 2025 15:25
@nikku nikku force-pushed the described-type-definitions branch from 9720389 to acfd390 Compare January 9, 2025 14:09
@nikku
Copy link
Member

nikku commented Jan 9, 2025

@MartyDashP I had a look at the PR, good bunch of TS magic! Find some adjustments pushed onto the branch, including proper integration into the overall library build + publishing.

I see that you heavily use expectType, which works, but in the end does not properly capture real world use. And these types, first and foremost have to be meaningful in the real world, hence we prefer that style of testing. Find one example here.

A few follow-ups we'd need if we wanted to incorporate it into the library:

  • The actual library (moddle) is not type-tested at all, and so isn't more complex descriptors
  • Optional vs. required properties: Right now every property is required (or always provided), which is not necessarily true (06a387d). We want to assess how that pans out in a real-world scenario, and if this is the right level of support we want to offer.

Probably next step is to get these real world examples incorporated (tests against moddle public API), or type some of our unit tests to verify current uses against the schema.

@MartyDashP MartyDashP requested a review from nikku January 11, 2025 15:37
@MartyDashP MartyDashP marked this pull request as ready for review January 11, 2025 15:39
@MartyDashP
Copy link
Author

MartyDashP commented Jan 11, 2025

@nikku I tried to cover all the public APIs with tests and hopefully took into account all the mentioned comments 🙂

@nikku
Copy link
Member

nikku commented Jan 20, 2025

Thanks for your updates. Hope to be able to look into this PR again, this week.

Co-authored-by: Nico Rehwaldt <git_nikku@nixis.de>
@MartyDashP MartyDashP requested a review from nikku February 20, 2025 15:15
@MartyDashP
Copy link
Author

@nikku 👋 Were you able to review my pull request? Do you have any comments or suggestions? 😀

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

Successfully merging this pull request may close these issues.

3 participants