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

chore(amplify-codegen): dart api model .fromJson() refactor #593

Merged
merged 3 commits into from
Apr 23, 2024

Conversation

Equartey
Copy link
Member

@Equartey Equartey commented May 11, 2023

Description of changes

Flatens the nested data structure of serializedData found in the .fromJson() method on codegen models. This PR is to be paired with changes in amplify-flutter#4665

Issue #, if available

aws-amplify/amplify-flutter#816

Description of how you validated changes

Change were validated via:

  1. amplify-codegen tests
  2. amplify-flutter api category unit & integ tests
  3. amplify-flutter datastore category unit & integ tests
  4. Manual testing within both categories on ios & android devices

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • Breaking changes to existing customers are released behind a feature flag or major version update
  • Changes are tested using sample applications for all relevant platforms (iOS/android/flutter/Javascript) that use the feature added/modified

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov-commenter
Copy link

Codecov Report

Merging #593 (9b143c7) into main (c9a5c3b) will increase coverage by 0.00%.
The diff coverage is 85.71%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff            @@
##             main     #593    +/-   ##
========================================
  Coverage   85.69%   85.69%            
========================================
  Files         148      153     +5     
  Lines        7380     7480   +100     
  Branches     1962     1976    +14     
========================================
+ Hits         6324     6410    +86     
- Misses        959      972    +13     
- Partials       97       98     +1     
Impacted Files Coverage Δ
packages/graphql-docs-generator/src/cli.ts 0.00% <0.00%> (ø)
...s/graphql-docs-generator/src/generator/generate.ts 91.66% <ø> (ø)
...es/amplify-codegen/src/utils/readSchemaFromFile.js 33.33% <33.33%> (ø)
packages/graphql-docs-generator/src/index.ts 70.90% <68.00%> (-14.10%) ⬇️
.../graphql-docs-generator/src/generator/getFields.ts 79.54% <80.00%> (+1.76%) ⬆️
...ackages/amplify-codegen/src/commands/statements.js 85.91% <81.48%> (-1.59%) ⬇️
...phql-docs-generator/src/generator/utils/loading.ts 86.36% <89.47%> (-2.53%) ⬇️
...ql-docs-generator/src/generator/utils/templates.ts 91.66% <91.66%> (ø)
.../amplify-codegen/src/callbacks/postPushCallback.js 92.85% <100.00%> (ø)
...mplify-codegen/src/callbacks/prePushAddCallback.js 62.16% <100.00%> (ø)
... and 14 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

….fromJson()

Allows decoding of non-transformed AppSync responses

aws-amplify/amplify-flutter#816
@Equartey Equartey force-pushed the chore/api-from-json-refactor branch from 54670ba to f337000 Compare April 19, 2024 16:42
dpilch
dpilch previously approved these changes Apr 19, 2024
indent(`.where((e) => e != null)`, 4),
indent(`.map((e) => ${this.getNativeType({ ...field, isList: false })}.fromJson(new Map<String, dynamic>.from(e)))`, 4),
indent(`.toList()`, 4),
indent(`: (json['${varName}'] as List)`),
Copy link
Member

Choose a reason for hiding this comment

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

Q: Can we be sure that json['${varName}'] is a List at this point?

The existing logic only casts to a List after checking that the type is List. The new logic casts to a List after checking that it is not null and not a Map. If json['${varName}'] were ever something other than Map, List, or null this would throw an exception.

The logic in the PR is:

  • If map, handle map
  • Otherwise, if not null, cast to List and handle list
  • Otherwise return null

Would it make more sense to have:

  • If map, handle map
  • Otherwise, if List, handle list
  • Otherwise, return null

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call out. I adjusted the logic to prefer type checks over null checks.

@dpilch dpilch merged commit f83eba1 into aws-amplify:main Apr 23, 2024
3 checks passed
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