-
Notifications
You must be signed in to change notification settings - Fork 87
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
Use metadata-booster #616
Conversation
There was a problem hiding this 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.
There was a problem hiding this 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", |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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
There was a problem hiding this 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! 🚀
Super @javiertoledo
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. |
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 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! |
If I understood what @NickSeagull said, the advantage of using @Command({ authorize: [Pepito, Juanito] })
export class Whatever {
...
} with this: command Whatever authorize Pepito, Juanito {
...
} As |
@alvaroloes : I would use
I definitely think that the step forward is to use |
@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! |
Closing as the changes here were included in another branch |
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:
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:
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
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
Additional information
This PR would unblock the following tasks: