feat(@aws-amplify/datastore): nested predicates on observe, observeQuery, TS fixes #9195

svidgen · 2021-11-10T23:42:07Z

Description of changes

This PR was initially about getting nested predicates to work for observe and observeQuery.

This effort snowballed into a few other areas as it brought my attention to a small mismatch with the design, which snowballed into other areas.

Here's what this includes:

Planned: Adds tests for observe and observeQuery.
Planned: Adds nested filtering in observe and observeQuery to work with new lazy loading behavior.
Mismatch fix: Changed getter for AsyncCollection to not be a promise.
Followup: Updates test models to use Promise/AsyncCollection and corrects associated tests.
Followup/Gap fill: Updates ModelInit to extract inner "settable" types and derive required-ness.
Followup/Gap fill: Removes DeepWritable. Updates are intended to force assignment to all top-level props instead of nested property updates.
Misc: Fixes incidental TS errors, seemingly the result of dep changes.

I had not planned on addressing TS gaps in this PR. That work was originally intended to be done with cascading saves. But, it seemed the path of least resistance to ensure we didn't leave any test models behind.

Issue #, if available

Description of how you validated changes

Checklist

PR description included
yarn test passes
Tests are changed or added
Relevant documentation is changed or added (and PR referenced)

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

…cements (aws-amplify#9141) * feat(@aws-amplify/datastore): constrain ObserveQuery options with explicit Type, add time interval/limit Promise race * feat(@aws-amplify/datastore): address PR callouts * feat(@aws-amplify/datastore): abstract SubscriptionBuffer to util class (TODO: add unit test) * feat(@aws-amplify/datastore): add unit test for util class * remove .only from test * feat(@aws-amplify/datastore): address PR callouts * remove .only from describe block * feat(@aws-amplify/datastore): PR-callouts, adjust naming conventions Co-authored-by: Sam Martinez <samlmar@amazon.com>

packages/datastore/__tests__/DataStore.ts

… when it is a killed state (aws-amplify#9055) * feat: new script to improve local dev experience for RN * feat: rn local dev script * docs: adds the needed documentation for running the local rn dev script * fix: pr review changes, formatting and code clean up * fix: refactored the open tab function and minor code cleanup * refactor: utility functions for lerna and wml cmd formation and added comments * fix: attempt to fix the lgtm bot issue to sanitized the path input * fix: attempt 2 to build cmds after sanitizing the path * fix: attempt three at solving the bot warning, sanatizes alias,npm bin and cd commands * refactor * fix: attempt four at fixing the lgtm warning * fix: clearer documentation on the usage of the script * refactor: change get delay and open new tab names * refactor: changed all caps wording to camel case for gotopackageroot element * feat: all flag to indicate script should use all supported packages * fix: adds a new UI packages list that is used to change their package name to directory name used in wml * refactor: adds a clear note to indicate script is different from the linking method of development * fix:handle reactContext is null by listening to changes * refactor: rephrase comment Co-authored-by: Chris F <5827964+cshfang@users.noreply.github.com> * refactor: emitNotificationOpened has been refactored to a new function and other nits * fix: adds an intent parameter to emitNotificationOpenedEvent * refactor: makes variables final Co-authored-by: Manoj NB <manojnb@amazon.com> Co-authored-by: Chris F <5827964+cshfang@users.noreply.github.com> Co-authored-by: Sam Martinez <samlmar@amazon.com> Co-authored-by: Ashish Nanda <ashish.nanda.5591@gmail.com>

This reverts commit 13e776e.

- @aws-amplify/ui-angular@1.0.33 - @aws-amplify/ui-components@1.9.4 - @aws-amplify/ui-react@1.2.24 - @aws-amplify/ui-storybook@2.0.24 - @aws-amplify/ui-vue@1.1.18 - @aws-amplify/analytics@5.1.4 - @aws-amplify/api-graphql@2.2.13 - @aws-amplify/api-rest@2.0.24 - @aws-amplify/api@4.0.24 - @aws-amplify/auth@4.3.14 - aws-amplify-angular@6.0.24 - aws-amplify-react-native@6.0.1 - aws-amplify-react@5.1.7 - aws-amplify@4.3.6 - @aws-amplify/cache@4.0.26 - @aws-amplify/core@4.3.6 - @aws-amplify/datastore-storage-adapter@1.1.12 - @aws-amplify/datastore@3.6.0 - @aws-amplify/geo@1.1.6 - @aws-amplify/interactions@4.0.24 - @aws-amplify/predictions@4.0.24 - @aws-amplify/pubsub@4.2.0 - @aws-amplify/pushnotification@4.3.3 - @aws-amplify/storage@4.4.7 - @aws-amplify/xr@3.0.24

codecov-commenter · 2021-11-12T16:56:17Z

Codecov Report

Merging #9195 (da57c65) into datastore-laziness (b2eaa3e) will increase coverage by 0.19%.
The diff coverage is 70.21%.

@@                  Coverage Diff                   @@
##           datastore-laziness    #9195      +/-   ##
======================================================
+ Coverage               78.91%   79.10%   +0.19%     
======================================================
  Files                     251      251              
  Lines                   18555    18619      +64     
  Branches                 3985     4046      +61     
======================================================
+ Hits                    14642    14729      +87     
+ Misses                   3784     3761      -23     
  Partials                  129      129

Impacted Files	Coverage Δ
packages/datastore/__tests__/helpers.ts	`100.00% <ø> (ø)`
packages/datastore/__tests__/model.ts	`100.00% <ø> (ø)`
...ckages/datastore/src/sync/datastoreConnectivity.ts	`32.25% <0.00%> (ø)`
packages/datastore/src/types.ts	`82.58% <0.00%> (ø)`
packages/datastore/src/sync/index.ts	`13.41% <8.33%> (ø)`
packages/datastore/src/sync/processors/sync.ts	`41.21% <18.75%> (ø)`
...ages/datastore/src/sync/processors/subscription.ts	`35.78% <37.83%> (-0.18%)`	⬇️
packages/datastore/src/sync/processors/mutation.ts	`65.62% <47.36%> (-0.35%)`	⬇️
...tastore/src/storage/adapter/AsyncStorageAdapter.ts	`82.96% <66.66%> (+0.28%)`	⬆️
packages/datastore/src/storage/storage.ts	`90.90% <69.23%> (-2.63%)`	⬇️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2eaa3e...da57c65. Read the comment docs.

…ws-amplify#9191) * feat: new script to improve local dev experience for RN * feat: rn local dev script * docs: adds the needed documentation for running the local rn dev script * fix: pr review changes, formatting and code clean up * fix: refactored the open tab function and minor code cleanup * refactor: utility functions for lerna and wml cmd formation and added comments * fix: attempt to fix the lgtm bot issue to sanitized the path input * fix: attempt 2 to build cmds after sanitizing the path * fix: attempt three at solving the bot warning, sanatizes alias,npm bin and cd commands * refactor * fix: attempt four at fixing the lgtm warning * fix: clearer documentation on the usage of the script * refactor: change get delay and open new tab names * refactor: changed all caps wording to camel case for gotopackageroot element * feat: all flag to indicate script should use all supported packages * fix: adds a new UI packages list that is used to change their package name to directory name used in wml * refactor: adds a clear note to indicate script is different from the linking method of development * chore: upgrade firebase package and made required changes to getToken and onNewToken * refactor: review comments, added exception handling to get token * Apply suggestions from code review Co-authored-by: Sam Martinez <sammartinez19@gmail.com> Co-authored-by: Chris F <5827964+cshfang@users.noreply.github.com> * refactor: formatting updates * refactor: formatting updates * remove react event listener so no multiples are registered and adds an error callback to get token Co-authored-by: Manoj NB <manojnb@amazon.com> Co-authored-by: Ashish Nanda <ashish.nanda.5591@gmail.com> Co-authored-by: Caleb Pollman <cpollman1@gmail.com> Co-authored-by: Sam Martinez <sammartinez19@gmail.com> Co-authored-by: Chris F <5827964+cshfang@users.noreply.github.com>

lgtm-com · 2021-11-15T19:33:53Z

This pull request introduces 1 alert when merging 77ac3c4 into 743bf8f - view on LGTM.com

new alerts:

1 for Expression has no effect

lgtm-com · 2021-11-15T23:21:12Z

This pull request introduces 1 alert when merging c2c2909 into 743bf8f - view on LGTM.com

new alerts:

1 for Expression has no effect

lgtm-com · 2021-11-16T20:44:07Z

This pull request introduces 1 alert when merging 866acc9 into 23ed2f3 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

- @aws-amplify/ui-angular@1.0.34 - @aws-amplify/ui-components@1.9.5 - @aws-amplify/ui-react@1.2.25 - @aws-amplify/ui-storybook@2.0.25 - @aws-amplify/ui-vue@1.1.19 - @aws-amplify/analytics@5.1.5 - @aws-amplify/api-graphql@2.2.14 - @aws-amplify/api-rest@2.0.25 - @aws-amplify/api@4.0.25 - @aws-amplify/auth@4.3.15 - aws-amplify-angular@6.0.25 - aws-amplify-react@5.1.8 - aws-amplify@4.3.7 - @aws-amplify/cache@4.0.27 - @aws-amplify/core@4.3.7 - @aws-amplify/datastore-storage-adapter@1.1.13 - @aws-amplify/datastore@3.6.1 - @aws-amplify/geo@1.1.7 - @aws-amplify/interactions@4.0.25 - @aws-amplify/predictions@4.0.25 - @aws-amplify/pubsub@4.2.1 - @aws-amplify/pushnotification@4.3.4 - @aws-amplify/storage@4.4.8 - @aws-amplify/xr@3.0.25

lgtm-com · 2021-11-17T19:10:20Z

This pull request introduces 1 alert when merging cecf0ae into 23ed2f3 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

)

aws-amplify#8967

- @aws-amplify/ui-angular@1.0.35 - @aws-amplify/ui-components@1.9.6 - @aws-amplify/ui-react@1.2.26 - @aws-amplify/ui-storybook@2.0.26 - @aws-amplify/ui-vue@1.1.20 - @aws-amplify/analytics@5.1.6 - @aws-amplify/api-graphql@2.2.15 - @aws-amplify/api-rest@2.0.26 - @aws-amplify/api@4.0.26 - @aws-amplify/auth@4.3.16 - aws-amplify-angular@6.0.26 - aws-amplify-react@5.1.9 - aws-amplify@4.3.8 - @aws-amplify/cache@4.0.28 - @aws-amplify/core@4.3.8 - @aws-amplify/datastore-storage-adapter@1.2.0 - @aws-amplify/datastore@3.7.0 - @aws-amplify/geo@1.1.8 - @aws-amplify/interactions@4.0.26 - @aws-amplify/predictions@4.0.26 - @aws-amplify/pubsub@4.2.2 - @aws-amplify/pushnotification@4.3.5 - @aws-amplify/storage@4.4.9 - @aws-amplify/xr@3.0.26

Co-authored-by: Nick Arocho <16296496+nickarocho@users.noreply.github.com>

lgtm-com · 2022-02-03T20:12:18Z

This pull request introduces 1 alert when merging fbd4274 into b2eaa3e - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

nickarocho · 2022-02-03T21:27:45Z

packages/datastore/__tests__/DataStore.ts

@@ -13,7 +13,8 @@ import {
 	PersistentModel,
 	PersistentModelConstructor,
 } from '../src/types';
-import { Model, Post, Metadata, testSchema } from './helpers';
+import { Comment, Model, Post, Metadata, testSchema } from './helpers';
+import { UpdatingHookContext } from 'dexie';


Maybe I'm missing it due to the many diffs in this file, but where is UpdatingHookContext being used? And what does it do?

That's a really good question ... I didn't actually add this. I thought it showed up during the merge/rebase. But, it's not used. I'll remove it!

nickarocho · 2022-02-03T21:31:00Z

packages/datastore/__tests__/DataStore.ts

 			expectType<PersistentModelConstructor<PersistentModel>>(model);
 			expectType<PersistentModel>(element);
 			expect(opType).toEqual('INSERT');
 			expect(element.field1).toEqual('Smurfs');
+			sub.unsubscribe();


Is unsubscribe called here for cleanup purposes? Or is it affecting the intended behavior of the model in the test?

I can't remember, honestly.

I have a vague memory that I added these partly to ensure that calling unsubscribe() didn't blow anything up — it would be called in the normal course of things. I wanted to ensure I mirrored correct/normal usage.

packages/datastore/__tests__/DataStore.ts

nickarocho · 2022-02-03T22:09:37Z

packages/datastore/__tests__/Merger.test.ts

@@ -1,5 +1,5 @@
 import { ModelMerger } from '../src/sync/merger';
-import { PersistentModelConstructor } from '../src/';
+import { PersistentModelConstructor, ModelInstanceMetadata } from '../src/';


Another large diff question: I don't seeModelInstanceMetadata being used in this test file. Why import it?

I'm really not sure why this was added. Artifact of the merge. Removing.

I'm a little surprised our TS Config doesn't catch unused imports 🤔

nickarocho · 2022-02-03T22:23:29Z

packages/datastore/src/datastore/datastore.ts

@@ -1164,14 +1203,14 @@ class DataStore {
 				throw new Error(msg);
 			}

-			if (typeof idOrCriteria === 'string') {
+			if (typeof idOrCriteria === 'string' && modelConstructor) {


This is probably just my lack of understanding of this specific method, but why check for modelConstructor if it isn't being passed anymore? (via old diff getModelDefinition(modelConstructor))

Yep. Seems to be cruft. Removing.

nickarocho · 2022-02-03T22:27:36Z

packages/datastore/src/datastore/datastore.ts

 			// TODO: abstract this function into a util file to be able to write better unit tests
 			const generateSnapshot = (): DataStoreSnapshot<T> => {
-				const isSynced = this.sync?.getModelSyncedStatus(model) ?? false;
+				const isSynced = this.sync?.getModelSyncedStatus(model) ?? true;


Why do we want isSynced to default to true here?

This appeared to be a non-impactful change outside of a little debugging session. But, I didn't intend to leave it swapped. Fixing.

nickarocho · 2022-02-03T22:33:16Z

packages/datastore/src/storage/adapter/AsyncStorageAdapter.ts

+				// case 'MANY_TO_MANY':
+				// 	// TODO: implement
+				// 	throw new Error('WRITE THIS CODE');


Bumping to remove.

nickarocho · 2022-02-03T22:35:07Z

packages/datastore/src/storage/adapter/IndexedDBAdapter.ts

+				// case 'MANY_TO_MANY':
+				// 	// TODO: implement
+				// 	throw new Error('WRITE THIS CODE');


Same bump to remove.

nickarocho · 2022-02-03T22:42:10Z

packages/datastore/src/util.ts

@@ -390,7 +387,7 @@ export const traverseModel = <T extends PersistentModel>(
 	// 				// Intentionally blank
 	// 				break;
 	// 			default:


Any clarification on this?

lgtm-com · 2022-02-03T23:17:44Z

This pull request introduces 1 alert when merging da57c65 into b2eaa3e - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

nickarocho

LGTM 👍 Thanks for fixing the nit comments and removing the lingering cruft. This was a beast of a PR, thanks for the great improvements, Jon! 🚢🍾

github-actions · 2023-02-05T02:02:47Z

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 *-help channels or Discussions for those types of questions.

svidgen commented Nov 11, 2021

View reviewed changes

packages/datastore/__tests__/DataStore.ts Outdated Show resolved Hide resolved

Samaritan1011001 and others added 8 commits November 11, 2021 11:39

feat(amplify-js): add CD support (aws-amplify#9171)

d30709f

fix: AWSAppSyncRealTimeProvider websocket init fix

13e776e

Revert "fix: AWSAppSyncRealTimeProvider websocket init fix"

b5819c5

This reverts commit 13e776e.

fix: AWSAppSyncRealTimeProvider websocket init fix (aws-amplify#9200)

99dbdda

chore: preparing release

8c174d0

chore(release): update version.ts [ci skip]

959d908

svidgen force-pushed the observe-query-testing branch from 8835c30 to 77ac3c4 Compare November 15, 2021 19:25

svidgen force-pushed the observe-query-testing branch from fadc51d to fd8e051 Compare November 16, 2021 16:33

svidgen changed the title ~~added baseline observeQuery tests; prep for nested predicate observeQ…~~ fix(@aws-amplify/datastore): nested predicates on observe + observeQuery; fix model getters Nov 16, 2021

ashika112 and others added 3 commits November 16, 2021 13:34

chore: preparing release

0515a53

chore(release): update version.ts [ci skip]

c316b3e

feat(@aws-amplify/datastore): hasOne CRUD improvements (aws-amplify#9239

d521d17

)

svidgen changed the title ~~fix(@aws-amplify/datastore): nested predicates on observe + observeQuery; fix model getters~~ feature(@aws-amplify/datastore): nested predicates on observe, observeQuery; fix getters, models, modelinit Nov 17, 2021

svidgen changed the title ~~feature(@aws-amplify/datastore): nested predicates on observe, observeQuery; fix getters, models, modelinit~~ feature(@aws-amplify/datastore): nested predicates on observe, observeQuery, TS fixes Nov 17, 2021

svidgen changed the title ~~feature(@aws-amplify/datastore): nested predicates on observe, observeQuery, TS fixes~~ feat(@aws-amplify/datastore): nested predicates on observe, observeQuery, TS fixes Nov 17, 2021

TreTuna and others added 4 commits November 18, 2021 16:05

fix(geo): update client-location to fix tslib issue (aws-amplify#9247)

47f29a3

aws-amplify#8967

chore: preparing release

6fcffba

chore(release): update version.ts [ci skip]

a68e148

svidgen and others added 19 commits February 3, 2022 11:49

fixed rebase mistake

72a38b6

fixed types

be7d47b

fixed modelinit and copyof typing

a21086a

renames; cruft cleanup

f1365c1

fixed LGTM, removed unused import

6feffa4

yet another attempt to fix TS errors ...

8b435b5

set alwaysStrict to find TS errors locally, fixed TS errors

2f639ad

fix: TS casting errors in sync engine

0eff5e8

upgrade dexie

fa306c2

stash TS strictness fixes

a2fd6b8

fixed all TS erros?git add .??

3e205d5

fixed TS for ds sqlite adapter

dc21f9a

removed accidentally added tsconfig

f9aa320

fixed LGTM unused variable inits

bfd4f80

comment cleanup

aa37489

Update packages/datastore/__tests__/DataStore.ts

3353b86

Co-authored-by: Nick Arocho <16296496+nickarocho@users.noreply.github.com>

removed some cruft

187870d

revert superfluous null check; more error message details

45ac573

fixed tests after rebase

fbd4274

svidgen force-pushed the observe-query-testing branch from 3f40c34 to fbd4274 Compare February 3, 2022 20:06

nickarocho reviewed Feb 3, 2022

View reviewed changes

cruft cleanup

da57c65

nickarocho self-requested a review February 3, 2022 23:40

nickarocho approved these changes Feb 3, 2022

View reviewed changes

svidgen merged commit c50e13f into aws-amplify:datastore-laziness Feb 3, 2022

svidgen deleted the observe-query-testing branch February 3, 2022 23:44

github-actions bot locked as resolved and limited conversation to collaborators Feb 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(@aws-amplify/datastore): nested predicates on observe, observeQuery, TS fixes #9195

feat(@aws-amplify/datastore): nested predicates on observe, observeQuery, TS fixes #9195

svidgen commented Nov 10, 2021 •

edited

Loading

codecov-commenter commented Nov 12, 2021 •

edited

Loading

lgtm-com bot commented Nov 15, 2021

lgtm-com bot commented Nov 15, 2021

lgtm-com bot commented Nov 16, 2021

lgtm-com bot commented Nov 17, 2021

lgtm-com bot commented Feb 3, 2022

nickarocho Feb 3, 2022

svidgen Feb 3, 2022

nickarocho Feb 3, 2022

svidgen Feb 3, 2022

nickarocho Feb 3, 2022

svidgen Feb 3, 2022

nickarocho Feb 3, 2022

nickarocho Feb 3, 2022

svidgen Feb 3, 2022

nickarocho Feb 3, 2022

svidgen Feb 3, 2022

nickarocho Feb 3, 2022

nickarocho Feb 3, 2022

nickarocho Feb 3, 2022

lgtm-com bot commented Feb 3, 2022

nickarocho left a comment

github-actions bot commented Feb 5, 2023

feat(@aws-amplify/datastore): nested predicates on observe, observeQuery, TS fixes #9195

feat(@aws-amplify/datastore): nested predicates on observe, observeQuery, TS fixes #9195

Conversation

svidgen commented Nov 10, 2021 • edited Loading

Description of changes

Issue #, if available

Description of how you validated changes

Checklist

codecov-commenter commented Nov 12, 2021 • edited Loading

Codecov Report

lgtm-com bot commented Nov 15, 2021

lgtm-com bot commented Nov 15, 2021

lgtm-com bot commented Nov 16, 2021

lgtm-com bot commented Nov 17, 2021

lgtm-com bot commented Feb 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Feb 3, 2022

nickarocho left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 5, 2023

svidgen commented Nov 10, 2021 •

edited

Loading

codecov-commenter commented Nov 12, 2021 •

edited

Loading