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

Use metadata-booster #616

Closed
wants to merge 4 commits into from
Closed

Use metadata-booster #616

wants to merge 4 commits into from

Conversation

alvaroloes
Copy link
Contributor

Description

This PR uses a Typescript plugin (aka. "transformer") to generate extra metadata for any class. For more information about what metadata is generated, check the transformer README: https://github.com/boostercloud/metadata-booster

This is the result of a vacation-hack I did this Monday that turned to have very good results.

Problem and motivation

During the development of Booster, we have faced several times the limitations that Javascript has with regard to its runtime reflection capabilities to know the types of the application types. This is expected because it is a non-typed language.
We expected Typescript to solve this but, as it is eventually compiled to Javascript, all types are also lost. However, by using the compiler option "emitDecoratorsMetadata" we could get some information about types, and only of those that are decorated with a decorator.
So, if you decorate a class (as we do with Commands or ReadModels), you will only get the type of each parameter of its constructor. This brings the following limitations:

  • If you declare a property that's not among the constructor parameters, you won't get anything about it.
  • You don't get property names
  • You don't get methods' metadata
  • And the most limiting factor: if your property has a type that contains type parameters, you will only get the metadata of the enclosing type, not of its parameters. So if the type is Array<string>, you will get just "Array"

All these limitations, especially the last one, impedes building the GraphQL schema correctly, blocking the progress of the framework in, mainly, two areas:

  1. ReadModel searches and filtering
  2. Allow commands to return anything, which in turn blocks the progress of a possible future of building CRUD-like applications with Booster Framework.

Possible solutions without any plugin

We've started searching for solutions for this more than a year ago, but we didn't find anything compelling. This months @adriangmweb and me started searching for solutions again, as he is working on changes to the GraphQL schema, but the only one we found was to force the user to add a lot of decorators. This is an example of a possible solution using more decorators

@Command(...)
class CreateUser {
  public constructor(
    readonly id: UUID,
    readonly name: string,
    @ElemTypes([String, Address]) // We need to make the user write the internal types of the Map
    readonly addresses: Map<string, Address>
    @ElemTypes(/* What do we put here? This would need to be recursive */)
    readonly groupedAddresses: Map<string, Array<Address>>
  ) {

  @Returns(User) // But what if it returns an array of users?
  public static handle(...): Promise<User> {
    // ...
  }
}

@HelperType // We need to decorate this class too, even if the decorator does nothing. If not, Typescript won't generate metadata
class Address {
  constructor(
    public firstName: string,
    public lastName: string,
    public country: string,
    @ElemTypes([State])
    public state: Set<State>,
    public postalCode: string,
    public address: string
  ) {}
}

So we would clutter the user project with a lot of superfluous decorators.

Solution with the plugin

With the plugin "metadata-booster", there is no need for any decorator. It generates detailed metadata (including recursive type parameters like Promise<Map<String, Set>>) for all classes, no matter if they are decorated or not. It also includes metadata for methods, so we would completely unblock the progress in the areas mentioned before.

Drawbacks

Typescript still doesn't support plugins out of the box. They probably will, you can check the progress in this issue. The way to use it is through TTypescript. It is a wrapper that uses the official Typescript installation you have in your project but executing any plugin you have defined in your tsconfig.json file.

At first, I didn't like this, but after trying it, it turned it to be a tiny change in code, it works pretty well, and, more importantly, the Booster user won't notice it.

This PR contains the needed changes to use TTypescript and the plugin "metadata-booster" (changes only in the package.json and tsconfig.json files) and some other changes to adapt to the metadata structure the plugins uses.

Checks

  • Project Builds
  • Project passes tests and checks
  • Updated documentation accordingly

Additional information

This PR would unblock the following tasks:

Copy link
Member

@javiertoledo javiertoledo left a comment

Choose a reason for hiding this comment

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

Nice change, I think there's a lot of interesting stuff we could do with transformers! Would it require new tests or there are no functional changes? Even if there are no functional changes, it would be nice to check that the code changed here is properly tested and create issues to make sure we eventually create them, there are some missing tests in the core yet.

Copy link
Member

@NickSeagull NickSeagull left a comment

Choose a reason for hiding this comment

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

Hey @alvaroloes , this is AWESOME! I always was super eager to use transformers because they'd solve a lot of things for us.

The only thing that I'd change is the usage of ttypescript for ts-patch, as the latter allows you to use transformers by injecting itself in the active TypeScript, so you don't have to change your IDE to use ttsc or the npm scripts. And most importantly, it allows you to create transformers in before the typechecking phase (program-transformers)

@@ -25,7 +25,9 @@ export const template = `{
"prettier": "^1.19.1",
"typescript": "^3.9.3",
"ts-node": "^8.6.2",
"@types/node": "^13.5.1"
"@types/node": "^13.5.1",
"ttypescript": "1.5.12",
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using ttypescript and having to use the explicit wrapper ttsc it's better to use ts-patch as it patches your current typescript with support for transformers, so the rest stays the same. Oh, and supports pre-typecheck transformers!

@@ -55,6 +55,7 @@
"mock-jwks": "^0.3.1",
"nock": "^13.0.4",
"sinon": "9.2.3",
"sinon-chai": "3.5.0"
"sinon-chai": "3.5.0",
"metadata-booster": "0.3.1"
Copy link
Member

Choose a reason for hiding this comment

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

I believe this package should live together with the framework? Also it should have the @boostercloud package namespace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer it not to live in the framework repository, as this can be used by anyone in any project (I mean the transformer code)

However, I agree with you that we can add the @boostercloud namespace. Let's see if I can change that

@@ -46,7 +48,7 @@
"watch:local": "nodemon --watch ../framework-provider-local/dist --watch ../framework-provider-local-infrastructure --watch dist --exec \"../cli/bin/run start -e local\"",
"lint:check": "eslint --ext '.js,.ts' **/*.ts",
"lint:fix": "eslint --quiet --fix --ext '.js,.ts' **/*.ts",
"compile": "tsc -b tsconfig.json",
"compile": "npx ttsc -b tsconfig.json",
Copy link
Member

Choose a reason for hiding this comment

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

Same here, better use ts-patch

Copy link
Contributor

@adriangmweb adriangmweb left a comment

Choose a reason for hiding this comment

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

Thanks! 🙏🏻 Looking forward to use this change for generating the new GraphQL schema! 🚀

@alvaroloes
Copy link
Contributor Author

Super @javiertoledo

_...Would it require new tests or there are no functional changes? Even if there are no functional changes, it would be nice to check that the code changed here is properly tested and create issues to make sure we eventually create them....

There no functional changes and I adapted the tests that checked the metadata in Commands and ReadModels so that they verify the new metadata structure.
I can add tests to verify the metadata with more complex types, but I think those tests should go in the transformer repo ("metadata-booster"), which currently has no tests (yep, it was a hacking day and I don't do tests out of a good will 😂 ).
I'll create all the missing tests

@alvaroloes
Copy link
Contributor Author

alvaroloes commented Feb 19, 2021

Ultra @NickSeagull The thing I didn't like about ts-patch was that the user (or us, somehow, in a package.json script) needs to execute the patching process. I mean, you install typescript and then you need to execute ts-patch install. Also, this needs to be done in the CI/CD too and, more importantly, every time you update typescript.
Those reasons are the ones that made me go for ttypescript: it is just a "facade" in front of the current typescript installation, so no extra steps need to be done. It seemed a simpler process IMO.

If you have other reasons (I saw the one about the program transformers, but I would think of that when we had a use case to solve), I willing to be convinced!

@javiertoledo
Copy link
Member

If I understood what @NickSeagull said, the advantage of using ts-patch is that it would allow us to create more advanced syntactic sugar, like replacing this:

@Command({ authorize: [Pepito, Juanito] })
export class Whatever {
  ...
}

with this:

command Whatever authorize Pepito, Juanito {
  ...
}

As ts-patch happens "before" the compiler, the module will "desugar" the DSL into the regular form with the annotation, so no changes in the core are needed. Not sure if it makes sense to switch to ts-patch for this specific use case though. Would it make sense to keep using ttypescript for this and add a second extension that uses ts-patch if we ever want to create our own typescript superset? And regarding Avaro's worries about having to "patch" the compiler, maybe we can just run the patch install as part of the new boost build command, so we don't really need to make the user be aware of this... I don't know if you can check that the patch is installed or not, but I guess that nobody will die if we install the patch every single time we build the project :trollface:

@NickSeagull
Copy link
Member

@alvaroloes : I would use ts-patch over ttypescript for other reasons apart from what @javiertoledo said about syntactic sugar:

  1. ts-patch is better maintained than ttypescript - I was working in a fork of ttypescript and its just spaghetti code. Their CI/CD doesn't pass tests, and they don't update the README in a while.
  2. ts-patch is used in production by NativeScript and many companies. The author is very responsive on issues, whereas ttypescript's author takes months, and the one to intervene is usually the one from ts-patch
  3. You don't have to change your IDE and npm scripts to use ttsc instead of tsc. Rather you just do it once when you initialize/clone the project, which can even be done with Git hooks, with tools like husky or as @javiertoledo suggested, with our boost build or even the boost new:project

I definitely think that the step forward is to use ts-patch.

@alvaroloes
Copy link
Contributor Author

alvaroloes commented Feb 23, 2021

@NickSeagull Ok, understood your reasons. I used ttypescript because the number of changes we need to do in Booster and in the user applications seems fewer. Plus, none of the reasons you posted have an impact on what the transformer does not the user (There have been no changes to the Typescript syntax, so the IDE's are not affected).

My concern is that patching typescript installation seems scary. It is like messing with global variables: You need to be sure you keep it in sync after any changes are done through other ways. If this is not fully achieved, there could be some moment in which the program compiles fine, but it fails in runtime because the metadata was not generated 😓

Anyway, I will give it a try to see how it works. Thanks!

This was referenced Mar 8, 2021
@adriangmweb adriangmweb mentioned this pull request Apr 13, 2021
3 tasks
@alvaroloes
Copy link
Contributor Author

Closing as the changes here were included in another branch

@alvaroloes alvaroloes closed this May 11, 2021
@alvaroloes alvaroloes deleted the use-metadata-booster branch May 11, 2021 10:36
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.

4 participants