-
Notifications
You must be signed in to change notification settings - Fork 819
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
Custom Transformers base support #1396
Custom Transformers base support #1396
Conversation
@ambientlight thanks for the PR. A top level question - could we have this functionality without a new CLI subcommand? For instance you've introduced a naming convention and configuration file so by having them specified in that location the CLI configuration is implicit. One of our design principles in Amplify is to reduce the API surface area, and in the CLI this means we try to introduce new commands only when absolutely necessary. Could you take a look at what the experience would be without a new command? |
@undefobj: definitely, that would make this PR quite trivial.
That would mean users just need to specify transformers module path inside the config like:
The motivation for the CLI is to give a nice sugar: install a set of transformer from NPM, scan and they will be register in amplify cli. My though was - it would be cool to have that some time later extended to show all available community maintained transformers in npm, and this subcommand as an early starting point. This can be quite handy in times, specifically I like the way it is done in fastlane. I guess this can be later refined into a separate amplify cli plugin @undefobj: So basically you are suggesting to throw everything away except the |
Ok great.
Ok I see the use case you're looking to support.
Not suggesting anything yet. Right now I just want to understand the design as part of our review and iteration process. @kaustavghosh06 @mikeparisstuff - My initial thought is if we were to do this as a CLI function for discoverability of published packages, we shouldn't limit it to just custom Transformers but also look at custom plugins like Amplify Video: https://aws.amazon.com/blogs/media/introducing_aws_amplify_video/ |
@undefobj: this would have been great! |
@ambientlight I think this is great and very close to what I had in mind. A few comments. Is there any reason we couldn't simplify the {
"transformers": [
"some-transformer-via-npm",
"file:///some/absolute/local/module"
]
} I think it will useful to allow targeting local modules that have not been distributed to NPM while dev'ing which is the intention of handling I can see why the |
@ambientlight #1346 adds support for transformers that do not extend the |
@mikeparisstuff:
good point. |
@ambientlight Yes let's go ahead and remove the CLI command for now and try to simplify the transform.conf.json file such that users can easily add custom transformers directly using the file. We are working on enhancing the CLI plugin support and this command would be a natural extension when that is ready. Without thinking too deeply, I think that a simplified interface for the transform.conf file such as
should work but I am open to other suggestions if you have them. There are number of requests for this functionality so it would be great to get this done. Thanks for the hard work. |
@ambientlight Would be great if you could update the PR based on @mikeparisstuff's comments. |
@kaustavghosh06: I expect to have this sorted out within the next 3 days, thank for waiting on this one |
@kaustavghosh06, @mikeparisstuff: let me know you thoughts on this change |
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.
Looks good to me overall. Let me think about this failure case where the user tries to specify a transformer that does not exist.
const CustomTransformer = imported.default; | ||
return CustomTransformer.call({}); | ||
} catch (error) { | ||
context.print.error(error); |
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'm torn between whether I think this should fail fast or quietly fail by ignoring.
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 thought about it and think we should fail fast here with a helpful error to help push users to do the right thing the first time. I can envision getting frustrated when my transformer isn't working even though the project is successfully compiling. Printing the error is useful but I may miss it (especially in a CI/CD environment).
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.
@ambientlight Can you please update this to fail fast when a transformer fails to load? Other than that I think this looks good. Excited to get this out there and start getting feedback from users.
We need to work on documentation for this feature. This section of documentation will be sorely outdated (https://aws-amplify.github.io/docs/cli/graphql#writing-custom-transformers). I can work on the docs and we can sync up to have the PRs release together.
@mikeparisstuff: sounds great, in terms of documentation will be also great for us to describe utility modules like |
@kaustavghosh06, @mikeparisstuff: do you guys need my further support to get this merged? documentation? |
Adding @SwaySway and @yuth |
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.
2 nit comments, what we can fix later, but please take a look at my comments and resolve them as you think.
@@ -139,6 +140,22 @@ async function migrateProject(context, options) { | |||
} | |||
} | |||
|
|||
function loadCustomTransformersConfig(transformerConfigPath, context) { |
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.
This is duplicated code, can't we reuse loadConfig
from transformConfig.ts
in graphql-transformer-core
? I miss the exist check for the file, BOM stripping, what we've at other places when reading JSON files.
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.
absolutely, missed that (there was no transformerConfig at the time this pull request were originally written)
try { | ||
const imported = require(modulePath); | ||
const CustomTransformer = imported.default; | ||
return CustomTransformer.call({}); |
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.
What do you think about separating the loading and execution of the custom transformer? The catch
handles the loading issues, but if an execution error happens, then the error message will be misleading.
removed redundant loadCustomTransformersConfig, separated custom transformers importing and executing
Any news on this one? Thanks! |
@BabyDino: supposedly, fixes have been done according to comments above, so let’s wait when guys have some time |
@ambientlight Of course, just checking. Is there any documentation in the works that we already can preview? Since @mikeparisstuff made a comment that the custom resolvers will be outdated. |
…transformers-cli # Conflicts: # packages/graphql-transformer-core/src/index.ts
You mean custom cloudformation resources will be deprecated by amplify so the one would actually need to go with custom transformer directly?
Not that I aware of. good point, I should add that. At this point add either a npm module name or absolute path of the custom module in your resource directory
|
Thanks for the clarification.
In a way, yes. The full story on our side is that after seeing the re:Invent talk from Rick Houlihan (https://www.youtube.com/watch?v=HaEPXoXVf2k) we are looking for a more advanced way of using GraphQL resolvers (and only targetting a single DynamoDB table from multiple types). So we were looking into this: https://aws-amplify.github.io/docs/cli-toolchain/graphql#custom-resolvers. Since this PR allows for more flexibility on this matter by creating our own transformer I would agree that the documentation for custom resolvers is deprecated. |
This gets off-topic and slightly opinionated, but he is talking so well in that video, that from the viewer persective, it feels like THE ONLY right way to go, especially with dynamoDB labeling it as best practices, while storing denormalized relational data with adjancency list pattern in a single table is often what we might want to do, I would personally wouldn't go to such extreme as having a sole single table for everything that gets as incomprehensible as those big companies he mentioned in a vid who have skilled dev teams will actually need to hire Rick himself to sort it out, how would you go maintaining all that I wonder day to day (unless doing that will save you thousands in dynamodb which would only apply to very-very large scale apps). Several tables incorporating the data that I expect to query together is a way for me, but I would agree on the point there should not be each individual table per each entityType we want to model. Actually, for now we don't really need to make a custom transformer to try out adjacency list. The simplest thing we can do is collapse attributes of two models into one (say UserPost), have them nullable respectfully in each item, and do the reverse GCIs in a same way as in https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/bp-adjacency-graphs.html with @key transformers. Let your application layer handle the normalization of the data you retrieve (collapsing plain list of user and comments into 1 data structure you need) and it can be way to go at the start. For now, relevant discussion has been at aws-amplify/amplify-category-api#398, or let's hypothetically say It would be great to have union-types compiled and mapped out to single table with app synch request/response VTL-templates doing the work of bringing the data to the form you need, but for me it would be hard to make any reasonable estimates of how long it would take to have a such stable transformer written and I have questions to how well this generalizes since it feels those highly-denormalized tables designs are very case specific (though I am absolutely not an expert here) |
where was this mentioned exactly? unable to find this discussion |
Sorry, my bad. He only said the documentation would be sorely outdated. |
@ambientlight Would be great if you could submit a PR for the docs - based on this PR out here (https://github.com/aws-amplify/docs) - preferably in this section - https://aws-amplify.github.io/docs/cli-toolchain/plugins#custom-graphql-transformers |
@kaustavghosh06: on it, do we also need to do the decent update to https://aws-amplify.github.io/docs/cli-toolchain/plugins#custom-graphql-transformers to explain how graphql-transformer-core works?, current it lacks details about things like splitting stacks in TransformerFormatter, usage of cloudform, etc. |
@ambientlight yes we need to do some documentation to that as well. If you could update that as well with what you think is good and add me to the PR as a reviewer (or link to it here) I'll look it over. We'd like to merge this PR soon and we're very excited and appreciative of the work you've put in. |
@ambientlight did you have a chance to work on the docs yet? how can I help? |
@attilah: I barely started extending pardon for having docs waiting for so long, what do you think if we just make a single paragraph explanation to
to have this closed and then the overall update to |
@attilah What do you think about @ambientlight's recommendation? I think we should merge the code and add a small snippet for it for now. |
@ambientlight that works for me, sounds like a plan. One thing came to my mind, what do you think of this modification: Since we've the new plugin system now, it would be convenient to automagically load transformers if a plugin contains one. It requires some addition to the plugin platform (@UnleashedMind could help there) so the loading of the transformers could happen by the platform itself, each plugin could expose a conventionally named factory method to instantiate the given transformer, so the transformer only needs to iterate over those platform provided list of factory functions to initiate them. This would remove the requirement of manually configure transformers in I would not halt the merging of this PR, but would consider it in a separate PR. Just rebase the files so we could merge it. |
Codecov Report
@@ Coverage Diff @@
## master aws-amplify/amplify-cli#1396 +/- ##
==========================================
+ Coverage 65.32% 65.59% +0.27%
==========================================
Files 198 200 +2
Lines 9708 9671 -37
Branches 1853 1973 +120
==========================================
+ Hits 6342 6344 +2
+ Misses 3094 3052 -42
- Partials 272 275 +3
Continue to review full report at Codecov.
|
This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs. Looking for a help forum? We recommend joining the Amplify Community Discord server |
Issue #, if available: #1060
Description of changes:
Introduces base cli support for custom transformers.
Approach:
amplify api configure-transformers
sub command has been added which allows users to scan/add/remove custom transformers packages.Details:
transform.conf.json
as mentioned in RFC - BYOTransformer #1060 thought the transformers have a following formattransform.conf.json
with custom non npm-package transformer specifying only{ enabled, modulePath }
, but such transformers will be ignored when viewing existing transformers by runningamplify api configure-transformers
Implementation Notes
graphql_transformer_manager.js
has been created inamplify-helpers
and is available oncontext.amplify.transformersManager
with base load/save-config, scan logicamplify-provider-awscloudformation/lib/transform-graphql-schema
will load custom transformers fromtransform.conf.json
when transformer info has enabled is set to true withrequire(transformer.modulePath)
Caveats
amplify_cli now depends on
graphql-transformer-core
(not sure if that's ideal) forThere are few issues with dynamically loading ts es5 emitted modules:
instanceof
to verify whether custom transformer module is a valid transformer, instead this hack is used:CustomTransformer()
. InsteadCustomTransformer.call({})
hack:This happen because dynamically imported
tsc
emitted es5 will havethis
undefined in_super.call(this, ...)
@mikeparisstuff: if you guys can advice on how this should be handled.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.