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

Switch JS/TS API wrappers to ES modules #456

Merged
merged 5 commits into from
Feb 29, 2024
Merged

Conversation

Whathecode
Copy link
Member

Previously, the JS/TS wrappers were compiled as UMD modules. This PR switches them to use the ES module system. The underlying generated libraries by Kotlin are still compiled as UMD.

This may make it easier to incorporate these libraries in modern web projects which use ES module systems by default. As such, this PR replaces PR #455.

@Whathecode Whathecode marked this pull request as draft February 13, 2024 07:31
@Whathecode
Copy link
Member Author

Whathecode commented Feb 13, 2024

There is still one piece missing to make this work. A package.json needs to be added to the generated libraries, otherwise they aren't recognized. Probably a more stringent requirement in the newer module system.

Adding a minimal pacakge.json, e.g., like this one, works:

{
  "name": "@cachet/carp-common-generated",
  "main": "index.js"
}

@Whathecode
Copy link
Member Author

Whathecode commented Feb 17, 2024

@jakdan99

I updated build.gradle to copy over package.json files and slightly modify them in line with other changes made to the JS sources in the build pipeline. Tests pass now.

You should be able to run verifyTsDeclarations (just like the build server does to test the JS/TS sources) and find/copy over the relevant packages from carp.core-kotlin\typescript-declarations\node_modules\@cachet.

Could you review please, and see whether the new generated sources work for you?

@Whathecode Whathecode marked this pull request as ready for review February 17, 2024 17:41
@Whathecode Whathecode requested a review from jakdan99 February 17, 2024 18:06
Copy link
Collaborator

@jakdan99 jakdan99 left a comment

Choose a reason for hiding this comment

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

Supressing these errors with @ts-nocheck for testing purposes, both the httpclient's tests and the dashboard runs successfully.

@jakdan99
Copy link
Collaborator

jakdan99 commented Feb 20, 2024

I could fix all of the above errors by copying Kotlin-DateTime-library-kotlinx-datetime-js-ir.d.ts, kotlinx-serialization-kotlinx-serialization-core.d.ts and kotlinx-serialization-kotlinx-serialization-json.d.ts files from carp-kotlinx-* folders to their respective folders and renaming them to index.d.ts. So if it can be done while generating the ts library every comment of mine can be resolved

@Whathecode
Copy link
Member Author

Whathecode commented Feb 22, 2024

So if it can be done while generating the ts library every comment of mine can be resolved

Thank you for finding the root cause! 🦾 The error messages indeed indicated missing declaration files, so presumed it was something going wrong in the lookup.

This should give me enough input to change the build pipeline, and hopefully it subsequently still passes with the build setup of this project. :)

Either way, looks super promising, and that this PR is the way to go then, instead of #455.

jakdan99
jakdan99 previously approved these changes Feb 27, 2024
Copy link
Collaborator

@jakdan99 jakdan99 left a comment

Choose a reason for hiding this comment

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

Now it builds perfectly. Great work!

`ts-node` and `jsdom` seem to be no longer needed when running mocha tests.
Also updated other JS/TS dependencies. Primarily, updating to nodenext also allowed to finally upgrade `@typescript-eslint/typescript-estree`, which depended on this module system.
Later more strict JS module systems require package.json to be present.
Without these, projects using the output modules did not automatically pick up the typescript declarations.
@Whathecode Whathecode merged commit e058fc8 into develop Feb 29, 2024
3 checks passed
@Whathecode Whathecode deleted the typescript-changes branch February 29, 2024 14:25
@Whathecode Whathecode mentioned this pull request Mar 18, 2024
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.

2 participants