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

setup for typescript, eslint, prettier git hooks & nx configuration #1139

Closed

Conversation

Laurin-W
Copy link
Contributor

I created configurations for typescript, eslint and nx in both the middleware and frontend packages.

NX

I added nx (which integrates well with lerna, the company behind nx took it over a while ago) but for the current use cases, I don't think there are any benefits right now.

Typescript

Typescript is set up to analyze the json files only. This way, we don't need to convert all files to .ts files. Types, interfaces, etc. can be defined in d.ts files and referenced in jsdoc /** @type annotations. This seems to be the least invasive way of progressing to me. An open question remains how/if to enforce stricter typing requirements for newly written code. This could be done, by for example adding stricter linting rules (e.g. warnings for usage untyped variables) or by allowing both typescript and javascript files in the project where javascript files are migrated step by step. For the latter case, a compilation to javascript needs to be set up (which is fairly easy though). The issue with adding stricter linting rules might be, that it can feel messy to have ~500 linting errors/warnings in the code base.

Eslint

With eslint, I tried to things: using the standard eslint:recommended rules and an extended and well-developed rule-set from airbnb.

  1. First, I tried to use the eslint:recommended + plugin:@typescript-eslint/recommended rules. That works fine, eslint generates quite some warnings and errors but at least the errors were fixable quite affordably. A couple of things seemed quite strange so I kept them untouched for someone else to have a second look at (just run yarn lint on the appropriate commit).
  2. With the airbnb rules, the number of eslint issues increased significantly. A large portion of them were autofixable (autofixable issues are mostly things like converting let declarations to const where applicable). In this PR I kept the airbnb configurations in separate commits and ran autofix (on the middleware project) as well. The issue here is that a lot of problems are not auto-fixable and it would probably take some time to rework all of them. So again, it's a question of taste and if it feels messy to have so many linting errors/warnings everywhere.

Prettier

The prettier integration with eslint works pretty well. Airbnb eslint rules come with styling guidelines as well. When adding prettier to the eslint configuration, prettier handles all that instead of airbnb.
In my editor (vscode) I enabled autoformatting and autofixing eslint issues on save. I can really recommend!

Pre-commit hooks

In analogy to the pre-commit prettier hooks in activitypods, I added that here as well.

Interested to hear what you think :)

@Laurin-W
Copy link
Contributor Author

Almost forgot about the kind of most important thing: typing support for services.

There are a couple of approaches, to achieve this.
In the most basic way, we can just define and use type annotations for our services, actions, settings, methods, ...
For the core service, I added a couple of annotations for illustration, see commit 3fa99c6
In service.d.ts, I added an interface that describes the service settings.

However, when calling a service action, the IDE can't help with providing available actions and the required parameters. There is a module called moleculer-ts, however this and other ts-moleculer projects seem to require the use of class-style service-definitions. That would be quite a major change, I haven't worked with rewrites so I don't know how much work something like that would be.

Anyways, I guess it's not necessary, as long as we write type definitions for the services, actions, settings since we can then import them. Not as neat but way better than tapping in the dark.

A method annotation might look something like that:

/** @type {import('moleculer').ServiceSchema<CoreServiceSettings>} */
const CoreService = {
  name: 'core',
  // Settings type is defined by CoreServiceSettings.
  settings: {},
  ....
        methods: {
          /**
           * @param {import('./service').MethodAuthenticateContext} ctx
           * @param {ApiGatewayService.Route} route
           * @param {IncomingMessage} req
           * @param {ServerResponse} res
           */
          authenticate(ctx, route, req, res) {
            if (req.headers.signature) {
              return ctx.call('signature.authenticate', { route, req, res });
            }
            if (req.headers.authorization) {
              return ctx.call('auth.authenticate', { route, req, res });
            }
            ctx.meta.webId = 'anon';
            return Promise.resolve(null);
          },

@Laurin-W
Copy link
Contributor Author

Oh, two more things:

  1. To actually enable static typechecks (not just code completion hints), in the .js files we need to add // @ts-check at the beginning of the file.
  2. It is possible to migrate the codebase to typescript files easily as well. Since typescript is a superset of javascript, that does not cause harm per se. You can gradually change file extensions from .js to .ts as you are working at something. Typescript compiles .ts files into a dist directory, .js files are compied. In a first attempt of doing so, I didn't encounter issues, running the compiled / copied code.
    I guess, it's a matter of taste in the end and if you like compiled files in a separate dist directory.

@srosset81
Copy link
Contributor

Thanks for the PR ! That's very precious.
I try to answer you quickly so that this PR can be merged soon and we avoid too much merge conflicts.

Typescript is set up to analyze the json files only. This way, we don't need to convert all files to .ts files. Types, interfaces, etc. can be defined in d.ts files and referenced in jsdoc /** @type annotations. This seems to be the least invasive way of progressing to me.

That sounds like a good option !
From what you say on another comment, it would be easy then to convert JS files with JSDoc to TS files ?

I don't think TS will be used soon on the frontend components. But I guess adding the TS config is not much expensive ?

An open question remains how/if to enforce stricter typing requirements for newly written code. This could be done, by for example adding stricter linting rules (e.g. warnings for usage untyped variables) or by allowing both typescript and javascript files in the project where javascript files are migrated step by step. For the latter case, a compilation to javascript needs to be set up (which is fairly easy though). The issue with adding stricter linting rules might be, that it can feel messy to have ~500 linting errors/warnings in the code base.

I wouldn't enforce anything with TS, while we do tests.

With eslint, I tried to things: using the standard eslint:recommended rules and an extended and well-developed rule-set from airbnb.

We can keep the airbnb rules and, if some of these rules are too hard, we can turn them off, like you already did for some of them. Linting is very useful, thanks for activating this feature !

Almost forgot about the kind of most important thing: typing support for services.
There are a couple of approaches, to achieve this.
In the most basic way, we can just define and use type annotations for our services, actions, settings, methods, ...
For the core service, I added a couple of annotations for illustration, see commit 3fa99c6
In service.d.ts, I added an interface that describes the service settings.

I have tried to see if autocomplete worked with the CoreService of Archipelago (with a yarn link to your branch of SemApps) but there was no suggestion in vscode. Did I miss something ? If there something more to do to make autocomplete work in Archipelago ?

However, when calling a service action, the IDE can't help with providing available actions and the required parameters. There is a module called moleculer-ts, however this and other ts-moleculer projects seem to require the use of class-style service-definitions. That would be quite a major change, I haven't worked with rewrites so I don't know how much work something like that would be.

Indeed this moleculer-ts package looks neat, but it doesn't seem easy to use, especially in our current configuration.

* @param {ApiGatewayService.Route} route
* @param {IncomingMessage} req
* @param {ServerResponse} res
*/
authorize(ctx, route, req, res) {
Copy link
Contributor

Choose a reason for hiding this comment

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

authorize and authenticate are two methods defined by the ApiGateway mixin. But I guess there is no way to guess them from this mixin ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, I am not aware of a way to guess the types from that mixin, unfortunately. This seems to be a typescript limitation right now, at least if the service configuration objects are not put in a function call straight away (see https://stackoverflow.com/questions/69976058/is-there-a-way-to-infer-a-generic-type-without-using-a-function).

The type definitions here were rather for demonstration purposes but since this becomes rather bloaty here, I'd probably leave it out?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes better to leave it out until Moleculer improves its type integration.

@Laurin-W
Copy link
Contributor Author

I have tried to see if autocomplete worked with the CoreService of Archipelago (with a yarn link to your branch of SemApps) but there was no suggestion in vscode. Did I miss something ? If there something more to do to make autocomplete work in Archipelago ?

Actually, I found a PR in moleculerjs/moleculer#829
This has been stale for some time but if it was approved, it could support ctx.call autocompletions at least for event and action definitions in the same service. Mixin actions, events, settings are not supported (this needs improvements on the typescript side which have been in discussion for a while).

@Laurin-W
Copy link
Contributor Author

Laurin-W commented Aug 24, 2023

Alright, I made some changes and got some remarks:

  • The linter now checks for typescript issues itself (so no need for @ts-check at the start of a file). All typescript issues are displayed as warnings when running the lint. That makes the number of warnings become very huge right now. Many of them can probably be eliminated by installing @types packages for untyped libraries. In most files you can now see warnings for potentially unsafe access of (e.g. any-typed) variables.
    EDIT: Many of those issues actually seem to arise because the linter (as well as my IDE) does not seem to be able to pick up the source information for some packages. Do you have those issues as well or have an idea where this is coming from?

  • If we define types in separate .d.ts files, we cannot reuse the same name for a .js file (e.g. index.js and index.d.ts). That's due to limitations between typescript and typescript-eslint. That's not a big issue though, just wanted to share since I ran into that and it took me a while to figure out what I was doing wrong.

  • From what you say on another comment, it would be easy then to convert JS files with JSDoc to TS files ?

    There are converters available for this. I haven't tried them out in practice but that seems to be an option.

  • I made the linter run as a pre-commit hook. The linter run usually takes ~5-10s. If errors are detected, the commit is aborted.

  • I ran autofix / prettier on all files in the project. That's why there are so many file changes. If that's to much right now, I can leave that out. Or maybe do it in another PR.

@srosset81
Copy link
Contributor

srosset81 commented Aug 31, 2023

The linter now checks for typescript issues itself (so no need for @ts-check at the start of a file). All typescript issues are displayed as warnings when running the lint. That makes the number of warnings become very huge right now. Many of them can probably be eliminated by installing @types packages for untyped libraries. In most files you can now see warnings for potentially unsafe access of (e.g. any-typed) variables.
EDIT: Many of those issues actually seem to arise because the linter (as well as my IDE) does not seem to be able to pick up the source information for some packages. Do you have those issues as well or have an idea where this is coming from?

For me there is now way too many linter issues for this to be useful. My IDE is cluttered with them. Typescript is something that should be implemented slowly over time, it is not a priority right now. I would prefer if the linter returned less issues, and we can fix them.

@Laurin-W
Copy link
Contributor Author

Laurin-W commented Sep 7, 2023

Totally see that. I commented out the typescript-rules of the configuration. I left the ts parser on, to avoid complications with the .d.ts files.

Now it's ~700 issues remaining for each frontend and middleware. I'd agree with most of them, if that's still too much, I could take an afternoon or two and address them. The majority of them is rather easy to fix, I think.

@srosset81
Copy link
Contributor

srosset81 commented Sep 11, 2023

Thanks for that. But I still get many typescript-related issues

Capture d’écran du 2023-09-11 11-30-26

@Laurin-W
Copy link
Contributor Author

Laurin-W commented Sep 11, 2023

Thanks for that. But I still get many typescript-related issues

Sorry, the tsconfig checkJs setting was still on which made the IDE complain.

@Laurin-W
Copy link
Contributor Author

Closing in favor of #1152

@Laurin-W Laurin-W closed this Sep 12, 2023
@Laurin-W Laurin-W deleted the chore/typescrit-eslint-nx branch September 12, 2023 07:20
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