feat/Support-apiId(Multiple-cloudformation-stacks) #3
Conversation
There was a problem hiding this comment.
Added comments from sid88in#597 (comment)
Thank you for your patience, I had a look at the PR. It looks good but I left some comments for improvements. Especially on typing and validation.
A few more thoughts/questions:
- should we allow exporting/importing data sources from other stacks?
e.g. I create my data sources in a main stack (could be a single-table DynamoDB), and then I reference it from resolvers in other stacks.
Right now, we use an intrinsic function to get the name, which will not work if it's in a different stack. I assume passing the value/name as-is should work.The same goes for pipeline functions.
- We also need to update the migration guide and remove the reference that using existing APIs is not supported since this is being re-introduced.
- I'd really like to have a full guide on what this is for and when and how to use it.
Thank you all for the effort!
src/resources/Api.ts
Outdated
| } | ||
|
|
||
| hasPipelineFunction(name: string) { | ||
| return name in this.config.pipelineFunctions; | ||
| return name in (this.config.pipelineFunctions || {}); |
There was a problem hiding this comment.
https://github.com/sid88in/serverless-appsync-plugin/pull/597/files#r1223313143
I understand why you made those optional, but I'd rather keep them as required. If you look here, those are actually already optional from a config point of view. Then,
getAppSyncConfig()makes sure to fill them with empty{}if needed for when it's injected in the compiler.
src/resources/Api.ts
Outdated
| endpointResource.Properties, | ||
| this.compileAuthenticationProvider(this.config.authentication), | ||
| ); | ||
| if (this.config.authentication) { |
There was a problem hiding this comment.
https://github.com/sid88in/serverless-appsync-plugin/pull/597/files#r1223301264
authenticationis always required in this context. There is probably some TS magic to do (i.e. a type union type or something)
src/index.ts
Outdated
| @@ -410,6 +421,10 @@ class ServerlessAppsyncPlugin { | |||
| async getIntrospection() { | |||
| const apiId = await this.getApiId(); | |||
|
|
|||
| if (typeof apiId !== 'string') { | |||
| return; | |||
There was a problem hiding this comment.
https://github.com/sid88in/serverless-appsync-plugin/pull/597/files#r1223290575
just returning is probably not good enough. I would show a warning.
In fact I would probably not allow calling any of these commands (assoc domain, etc) from a "child" service stack.
Please call this command from the service where the AppSync API is created.
src/index.ts
Outdated
| @@ -377,6 +384,10 @@ class ServerlessAppsyncPlugin { | |||
| async gatherData() { | |||
| const apiId = await this.getApiId(); | |||
|
|
|||
| if (typeof apiId !== 'string') { | |||
There was a problem hiding this comment.
| if (typeof apiId !== 'string') { | |
| if (!apiId) { |
https://github.com/sid88in/serverless-appsync-plugin/pull/597/files#r1223292215
if for some reason,
apiIdis an empty string, it probably does not make sense to continue either.
| > Note: you should never specify this parameter if you're managing your AppSync through this plugin since it results in removing your API. | ||
|
|
||
| ### Schema | ||
| After specifying this parameter, you need to manually keep your schema up to date or from the main stack where your root AppSync API is defined. The plugin is not taking into account schema property due to AppSync limitation and inability to merge schemas across multiple stacks |
There was a problem hiding this comment.
https://github.com/sid88in/serverless-appsync-plugin/pull/597/files#r1223263789
This whole section is confusing and probably not necessary.
Instead, we should add clear guidelines on how and why you should use the
apiIdparameters. A bit like described here for API Gateway.
What are the main changes ?
What else changed in the project ?