fix: update TypeScript support and type definitions

[pvlugter]

Refs https://github.com/lightbend/akkaserverless-framework/issues/689. Fixes #134.

Add a TypeScript implementation of the TCK to test types. Updated for some missing or broken type declarations when going through this. The types for replicated entities still need some additional work.

[pvlugter]

Also resolved the remaining warnings from tsd-jsdoc.

[janory]

@pvlugter first of all thanks for the effort to fix the broken types! 💪

In my opinion our current mixed approach to generate the types makes really hard to get things done correctly.

By mixed approach I mean the following

• we are using the JSDoc annotations to generate an index.d.ts file which is the main entry point for the users of this library
• we have migrated some parts of the code to TS and therefore the npm run compile command emits the .d.ts files inside the dist/src library, but if the given codebase is not migrated to TS yet, the tsc compiler will try to find out the types based on the JSDoc annotations

So we are using 3 different ways to generate the TS types.
On top of that we are monkey patching the index.d.ts file which was generated by the jsdoc tool to include types from the emitted .d.ts files by the tsc compiler, because jsdoc can't parse the files which are already migrated to TS and otherwise those types would be always missing.

I think the outcome is something what is quite chaotic and hard to understand and maintain, on the other hand breaks very easily.

It somehow also gives the false assumptions that the generated types are correct, but I've looked into the index.d.ts file of the packed package and I've got lots of errors.
Also the emitted d.ts files by the tsc compiler have some errors.

To verify these errors please run the following commands:
tsc dist/index.d.ts --noEmit -> this validates the main entrypoint which is basically used by our users
tsc dist/src/*.ts --noEmit -> this validates the d.ts files which are generated by the tsc compiler either from the .ts files or based on the .js files' JSDoc, some of them are relevant for our users, some of them are not, depending what are patching into the index.d.ts file

So although the new TS TCK passes, there will be some type errors in the released library, that's why I think our publishing process should include those two commands mentioned above, to make sure that we won't release broken types from now on.

Regarding the TCK, I was thinking if wouldn't be enough to have only one version of it, which is written it TS?
Maybe I am missing something here, but since "TypeScript is a superset of JavaScript that compiles to clean JavaScript output." if the TS TCK would pass, it would be 100% sure that the underlying JS logic is correct as well.

Based on my concerns, my suggestion would be the following:

• Until we migrated everything to TS rely only the JSDoc when generating the types, no mixed approach, the tsc compiler would be only used to validate the existing .ts files and generated the js files but not for emitting .d.ts files.
• Make sure that the generated index.d.ts is always valid by running the tsc dist/index.d.ts --noEmit command before publishing
• Remove the JS TCK, since what it does is redundant with the TS TCK

If for some reason we want to keep the current mixed approach, we should at least make sure that we only emit the d.ts files for the .ts files, but not for the .js files.
To achieve this we could split up our compilation process into two steps:
tsc --allowJs false --checkJs false -> to compile only the .ts file along with the .d.ts files
tsc --declaration false --composite false -> to add the rest of the .js files without the .d.ts files

To make this work the settings.js should be migrated to settings.ts like this:

export const frameworkVersion = '0.7.0-beta.14'

export const protocolVersion = function () {
    const versions = frameworkVersion.split(/[.-]/);
    return {major: versions[0], minor: versions[1]};
}
export const baseVersion = function () {
    const version = protocolVersion();
    return `${version.major}.${version.minor}`;
}

and the include line should be also changed accordingly in the tsconfig.json.

To help you to fix the broken index.d.ts file I've manually created a version of it which compiles without an error.
We can diff this one with the generated one to find out what needs to be changed.
I think some of the errors are happening, because we have just dropped some of the JSDoc annotations too easily.
Dropping the JSDoc annotations harms the generated documentation as well, so I think we should be more careful doing so in the future.

Since it's a pretty long comment with lot of thoughts, we could have a direct feedback loop with each other to discuss the details, ping me Slack whenever it works for you.

[pvlugter]

In my opinion our current mixed approach to generate the types makes really hard to get things done correctly.

By mixed approach I mean the following

• we are using the JSDoc annotations to generate an index.d.ts file which is the main entry point for the users of this library
• we have migrated some parts of the code to TS and therefore the npm run compile command emits the .d.ts files inside the dist/src library, but if the given codebase is not migrated to TS yet, the tsc compiler will try to find out the types based on the JSDoc annotations

So we are using 3 different ways to generate the TS types.
On top of that we are monkey patching the index.d.ts file which was generated by the jsdoc tool to include types from the emitted .d.ts files by the tsc compiler, because jsdoc can't parse the files which are already migrated to TS and otherwise those types would be always missing.

I think the outcome is something what is quite chaotic and hard to understand and maintain, on the other hand breaks very easily.

Right. I haven't changed the approach at all, just looked to have more of the types covered, and checking usage from a project.

The plan is still to migrate fully to TypeScript, so we just want something in the interim that works too.

It somehow also gives the false assumptions that the generated types are correct, but I've looked into the index.d.ts file of the packed package and I've got lots of errors.
Also the emitted d.ts files by the tsc compiler have some errors.

To verify these errors please run the following commands:
tsc dist/index.d.ts --noEmit -> this validates the main entrypoint which is basically used by our users
tsc dist/src/*.ts --noEmit -> this validates the d.ts files which are generated by the tsc compiler either from the .ts files or based on the .js files' JSDoc, some of them are relevant for our users, some of them are not, depending what are patching into the index.d.ts file

So although the new TS TCK passes, there will be some type errors in the released library, that's why I think our publishing process should include those two commands mentioned above, to make sure that we won't release broken types from now on.

Sounds good. Good if we can verify the types easily.

Regarding the TCK, I was thinking if wouldn't be enough to have only one version of it, which is written it TS?
Maybe I am missing something here, but since "TypeScript is a superset of JavaScript that compiles to clean JavaScript output." if the TS TCK would pass, it would be 100% sure that the underlying JS logic is correct as well.

Yes, let's do that. I may not keep this PR as ongoing, but set that up later. The proxy also depends on the TCK implementation here (as a docker image) for integration tests.

Based on my concerns, my suggestion would be the following:

• Until we migrated everything to TS rely only the JSDoc when generating the types, no mixed approach, the tsc compiler would be only used to validate the existing .ts files and generated the js files but not for emitting .d.ts files.

Won't we be missing some types at the top-level, for those parts that are converted to TypeScript already? Or do we add just jsdoc to cover those as well? I'm not that familiar with JS or TS, but I think a mixed approach is fine while we're still migrating over to TypeScript, providing there are ways to make it work out.

• Make sure that the generated index.d.ts is always valid by running the tsc dist/index.d.ts --noEmit command before publishing

Sounds good.

• Remove the JS TCK, since what it does is redundant with the TS TCK

Yep, let's follow up with that.

If for some reason we want to keep the current mixed approach, we should at least make sure that we only emit the d.ts files for the .ts files, but not for the .js files.
To achieve this we could split up our compilation process into two steps:
tsc --allowJs false --checkJs false -> to compile only the .ts file along with the .d.ts files
tsc --declaration false --composite false -> to add the rest of the .js files without the .d.ts files

I don't think we want to continue with a mixed approach for long, just while we're still to migrate everything to TypeScript. Tidying this up sounds good.

To make this work the settings.js should be migrated to settings.ts

I think settings.js is also read directly in other places, to get the framework version, so let's make sure that works ok too.

To help you to fix the broken index.d.ts file I've manually created a version of it which compiles without an error.

Ok, let's follow up separately.

I think some of the errors are happening, because we have just dropped some of the JSDoc annotations too easily.
Dropping the JSDoc annotations harms the generated documentation as well, so I think we should be more careful doing so in the future.

Yes, we certainly still want API docs. I'm assuming that we'll be using tsdoc instead once we've fully migrated to TypeScript. While going through the migrating, if we can keep jsdoc comments and the API docs intact, that would be good.

Since it's a pretty long comment with lot of thoughts, we could have a direct feedback loop with each other to discuss the details, ping me Slack whenever it works for you.

I don't want to work on one continuous PR for all the TypeScript support, so let's move these into separate issues to follow up on. I think we want to focus on getting to fully TypeScript to make everything easier, but useful if we can keep types and API docs working during the transition.

I'm also working in other areas, so suggest that we merge this PR and build on it.

[pvlugter]

@janory, I've added verification for the types, and fixed the remaining broken types. These changes are to keep the mixed approach for now. See f983a1a. The ReplicatedEntityServices is an odd one in there — I'll come back to that one.

[pvlugter]

Have replaced the JavaScript implementation of the TCK with the new TypeScript implementation now.

[pvlugter]

#135 also updates the TCK for replicated entities. Either that PR or this one will need to be updated, depending on which is merged first.

[janory]

Thanks for taking the time to react to my comments and change the PR accordingly. 🙏
I think what we have achieved here is really great improvement and gives us a good migration path (and more confidence) for the rest of the code in the future. 💪

Nothing to fix in this PR, but after npm install I've realised that the package-lock.json got regenerated.
I've created a ticket for this issue: #154

[janory]

Suggested change

	pushd tck && npm install && npm run tck && popd
	pushd tck && npm ci && npm run tck && popd

[pvlugter]

Can't be npm ci because the package-lock.json is not committed. This was removed/ignored for the TCK a while back — I think because the dependency on the package is in the package.json and these end up conflicting with cached sha hashes each time it gets rebuilt locally.

[janory]

Why do we want to ignore this?
Keeping the package-lock.json makes sure that the dependency resolution is consistent and running the same code always gives back the same results.

[pvlugter]

Not completely sure, but I think it was because the dependency to the sdk in the package.json is now directly to the package (tgz file), and the sha for this keeps changing and causing issues. I certainly got those problems in this PR in CI on the first builds. I think it used to link to the sdk directory, but now uses the package.

[pvlugter]

This change was in #77

[janory]

Ahh got it.
Yeah, unfortunately we won't be able to keep the package-lock.json if the hash of the sdk is always dynamically changing with each build.

There is only one solution what comes to my mind:
We could use husky to create a pre-commit hook which always packages the sdk before a commit and uses the new hash to monkey patches the package-lock.json.
But it would the actual commit would generate a new outgoing change and it slows things down, so I am not sure that this is the right answer to our problem. It feels very dirty.... 😬
Apparently others had this problem as well: npm/cli#517 🤷‍♂️

But this is not strictly related to this PR, so nothing what we would need to solve here.

[pvlugter]

@janory, I've implemented your suggestion of only having tsd-jsdoc generate the types. See 8cd6928.

This reintroduces some jsdoc comments that were removed when files were converted to TypeScript, and extends to cover the user-facing types properly. These are added to *.jsdoc files (alongside each converted TS file) which are only used for generating the API docs and index.d.ts. No other TS definition files are generated now. And the doc comments in the TS files have been converted to TSDoc format, so we can use a TypeScript API docs generator once we've fully migrated over.

The advantage is that we get the API docs back for the converted parts. We also have all the types in the index definition file again. The disadvantage is that these types are duplicated for the TS files. And we'll need to maintain these doc comments for now, but shouldn't be a problem if we don't change them much while we're still transitioning.

[janory]

@janory, I've implemented your suggestion of only having tsd-jsdoc generate the types. See 8cd6928.

This reintroduces some jsdoc comments that were removed when files were converted to TypeScript, and extends to cover the user-facing types properly. These are added to *.jsdoc files (alongside each converted TS file) which are only used for generating the API docs and index.d.ts. No other TS definition files are generated now. And the doc comments in the TS files have been converted to TSDoc format, so we can use a TypeScript API docs generator once we've fully migrated over.

The advantage is that we get the API docs back for the converted parts. We also have all the types in the index definition file again. The disadvantage is that these types are duplicated for the TS files. And we'll need to maintain these doc comments for now, but shouldn't be a problem if we don't change them much while we're still transitioning.

@pvlugter this is truly an awesome job!
I am happy that you eventually decided against the mixed approach, because we gained back some valuable JSDoc comments and now with the verify-types command we can make sure that we won't break the exposed types accidentally! 👍
I agree that the maintenance should be a big issues, since we are not changing the SDK or introducing new types, but migrating existing logic to TS which shouldn't affect the JSDoc comments too much.

[janory]

Since we don't need two versions of the tsc command anymore, we can move the declaration and composite flags to the tsconfig.json config file.

[pvlugter]

Good idea. Done.