feat: generate api code #269
Conversation
🦋 Changeset detectedLatest commit: 138da96 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
dpilch
force-pushed
the
generate-api-code
branch
4 times, most recently
from
689bdf3 to
faea5eb
Compare
dpilch
force-pushed
the
generate-api-code
branch
from
faea5eb to
77feaa6
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
dpilch
force-pushed
the
generate-api-code
branch
2 times, most recently
from
fff9686 to
4963b85
Compare
dpilch
force-pushed
the
generate-api-code
branch
from
4963b85 to
4af3bd8
Compare
dpilch
requested review from
edwardfoyle,
sobolk and
Amplifiyer
as code owners
packages/model-generator/API.md
Outdated
| // @public (undocumented) | ||
| export type GeneratedOutput = { | ||
| [filename: string]: string; | ||
| }; |
There was a problem hiding this comment.
this seems to be unused. please hide.
There was a problem hiding this comment.
The is the return type of generateApiCode.
There was a problem hiding this comment.
hmm.
In that case shouldn't generateApiCode just return GenerationResult like the wrapped operations do?
Do we have use case where in-memory representation would be consumed?
There was a problem hiding this comment.
I'll update to GenerationResult. The in-memory functionality was a requirement from Rene and is how Al's PR is built https://github.com/aws-amplify/samsara-cli/pull/265/files#diff-dcea9a2f178bf1deb339fd2cc2aa95b9cf136b2f32a660c1c3501f8bd62c399dR30-R57
There was a problem hiding this comment.
Al's PR can use GenerationResult.writeToDirectory() API. Which was created for that purpose.
@alharris-at do you know why PMs want in-memory representation? Can this be descoped for now until we know what is the use case?
Regardless of whether it's a must or not the GenerationResult was a place to add Record<string, string> prop to provide that. But if we don't know use case we should hold.
There was a problem hiding this comment.
I'm fine chopping this out for now.
packages/model-generator/API.md
Outdated
| // @public (undocumented) | ||
| export type GenerationResult = { | ||
| writeToDirectory: (directoryPath: string) => Promise<void>; | ||
| operations: Record<string, string>; |
There was a problem hiding this comment.
Do we have a use case for this?
If not please hide.
| @@ -12,7 +12,7 @@ export class AppsyncGraphqlGenerationResult implements GenerationResult { | |||
| * @param operations A record of FileName to FileContent | |||
| * in the format of Record<string,string> | |||
| */ | |||
| constructor(private operations: ClientOperations) {} | |||
| constructor(public operations: ClientOperations) {} | |||
There was a problem hiding this comment.
This is so that generateApiCode can return the generation results without writing to the file system.
| maxDepth: 3, | ||
| typenameIntrospection: true, |
There was a problem hiding this comment.
Will dropping these defaults impact form gen, or is Spencer already passing these in?
There was a problem hiding this comment.
I don't think this is being used by the form generation yet. @sdstolworthy
| target: props.typeTarget, | ||
| multipleSwiftFiles: props.multipleSwiftFiles, | ||
| }); | ||
| return { ...documents, ...types }; |
There was a problem hiding this comment.
I need to fix this to be a new combined GenerationResult.
| // @public | ||
| export const generateApiCode: (props: GenerateApiCodeProps) => Promise<GenerationResult>; | ||
|
|
||
| // @public (undocumented) | ||
| export type GenerateApiCodeProps = GenerateOptions & BackendIdentifier & { | ||
| credentialProvider: AwsCredentialIdentityProvider; | ||
| }; |
There was a problem hiding this comment.
One more suggestion.
Rest of APIs are explicit about Graphql
Should we use generateGraphqlApiCode (and similar in props/return) ?
There was a problem hiding this comment.
Rene wanted customer facing APIs to use "API" over "Graphql". We'll be syncing with him on naming next week to finalize.
There was a problem hiding this comment.
Yeah, we'll be able to close on this by EOD Monday, but I wouldn't make it block this, since we want this in for the pentest ready date, which is also monday (easy to change)
There was a problem hiding this comment.
Also. Then
If this is customer facing I suggest to consider moving other types to namespaces.
So that import * as generator from '@aws-amplify/model-generator' does not cause explosion in IDE when customer types generator. .
For example other types can be moved to @aws-amplify/model-generator/documents @aws-amplify/model-generator/types what not. We do that in other packages that are customer facing like here:
https://github.com/aws-amplify/samsara-cli/blob/4fd18b126ba075d054ddc4b34859e67e519355e5/packages/client-config/package.json#L8-L18
Issue #, if available:
Description of changes:
generateAPICode).createGraphqlDocumentGeneratorandcreateGraphqlTypesGeneratorto use backendIdentifier and credentialProvider.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.