Provide parentType to addMany subscriptions #678

Joklost · 2022-11-05T12:23:07Z

Partially fixes #360

The parentType in addFieldSubscription was hardcoded to 'asdf'. This causes issues when using multiple nested @list directives, as using the [ListName]_(insert|remove|toggle) operations with @parentID attempted to look for a parent with type 'asdf'.

To help everyone out, please make sure your PR does the following:

Update the first line to point to the ticket that this PR fixes
Add a message that clearly describes the fix
If applicable, add a test that would fail without this fix
Make sure the unit and integration tests pass locally with pnpm run tests and cd integration && pnpm run tests
Includes a changeset if your fix affects the user with pnpm changeset

@list

The parentType in addFieldSubscription was hardcoded to 'asdf'. This causes issues when using multiple nested @list directives, as using the [ListName]_(insert|remove|toggle) operations with @parentID attempted to look for a parent with type 'asdf'. Signed-off-by: Jonas Jacobsen <[email protected]>

Route for testing nested lists has been added to e2e/sveltekit, Modles have been added to the e2e/_api GQL schema, and a simple "database" with sample data has been added to the resolvers. Signed-off-by: Jonas Jacobsen <[email protected]>

changeset-bot · 2022-11-05T12:23:10Z

🦋 Changeset detected

Latest commit: 3a9f2b5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
houdini	Patch
houdini-react	Patch
houdini-svelte	Patch

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

vercel · 2022-11-05T12:23:11Z

@Joklost is attempting to deploy a commit to the HoudiniGraphQL Team on Vercel.

A member of the Team first needs to authorize it.

Signed-off-by: Jonas Jacobsen <[email protected]>

Joklost · 2022-11-05T13:31:09Z

Any suggestions for the End-to-End (windows-latest) failing?

Error: Timed out waiting 60000ms from config.webServer.

  70 skipped
  1 error was not a part of any test, see above for details
Notice:   70 skipped
undefined
D:\a\houdini\houdini\e2e\sveltekit:
 ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL  [email protected] tests: `playwright test `
Exit status 1
Error: Process completed with exit code 1.

jycouet · 2022-11-05T13:58:51Z

@Joklost I don't know why it timed out. I just relaunched it, and it seems good now.

Maybe in e2e tests, you could move your operations files under src/routes/stores/nested-list/*. We are moving more and more to this style to have things more colocated.

I don't have time to really look now the PR, but it looks really promising already. Thx a lot for the effort. 🥳
@AlecAivazis could you 👀?

Joklost · 2022-11-05T14:07:14Z

@Joklost I don't know why it timed out. I just relaunched it, and it seems good now.

Hmm, alright - thanks!

Maybe in e2e tests, you could move your operations files under src/routes/stores/nested-list/*. We are moving more and more to this style to have things more colocated.

Sounds good - I will move them

Operation files only used by src/routes/stores/nested-list are now placed appropriately. Signed-off-by: Jonas Jacobsen <[email protected]>

AlecAivazis · 2022-11-05T19:34:21Z

packages/houdini/src/runtime/cache/subscription.ts

@@ -175,14 +176,15 @@ export class InMemorySubscriptions {
 					key,
 					selection: fieldSelection,
 					spec,
-					parentType: 'asdf',


🤦‍♂️

AlecAivazis · 2022-11-05T19:38:50Z

Thanks for updating this! The referenced ticket won't be fixed by this unfortunately because the issue is a bit more general - the full page parameters won't be sent with the query so loading one page in a list will destroy the other one

AlecAivazis · 2022-11-05T19:39:37Z

e2e/sveltekit/src/routes/stores/nested-list/+page.svelte

@@ -0,0 +1,98 @@
+<script lang="ts">


Wow!! I really appreciate this test. Super thorough!

vercel · 2022-11-05T19:40:06Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
docs	✅ Ready (Inspect)	Visit Preview	Nov 6, 2022 at 10:16PM (UTC)
docs-next	✅ Ready (Inspect)	Visit Preview	Nov 6, 2022 at 10:16PM (UTC)

AlecAivazis · 2022-11-05T19:40:38Z

packages/houdini/src/runtime/cache/subscription.ts

 					// insert the subscriber
 					this.addMany({
 						parent: linkedRecord,
 						selection: fields,
 						variables,
 						subscribers,
+						parentType: linkedType,


Actually, could you add a test that covers this?

As far as I understand, this is already covered in the e2e page, as this is what fixed the innermost list (Books) in the test.

If more is needed, I'll definitely take a look.

yea your e2e definitely covers it but I'd like to duplicate the verification. the cache is a big risk in when refactoring so i'd like to make sure the test suite covers as much as possible so we can get quick feedback when working on it

Alright, I'll look into it - thanks

thank you for the time!

Hi,

I have added a pretty verbose, but simple, test in 3a9f2b5. I am pretty new to these frameworks, so I just attempted to mimic the behaviour I saw when running the e2e test, and added it here.

Please let me know if I should further expand the test.

No worries at all about the verbosity! I'd rather that than have no test at all. I'm going to pull down your PR and confirm that test fails without your fix and then it should be good to go. Thanks again!

PS: I might need you to reopen the PR to kick off the CI/CD stuff. Github Actions have been struggling recently

Adds a simple test for testing data with nested lists. The test adds the cities/books/libraries database to the cache, and adds first a city, then a library. Next, a subscription is added, before adding a book to the cache, and it is verified that the book is added to the library. Signed-off-by: Jonas Jacobsen <[email protected]>

AlecAivazis · 2022-11-07T18:26:24Z

Damn, doesn't look like that did it. Mind closing this PR and opening another one?

Joklost added 2 commits November 5, 2022 12:49

e2e testing of nested lists

57b3777

Route for testing nested lists has been added to e2e/sveltekit, Modles have been added to the e2e/_api GQL schema, and a simple "database" with sample data has been added to the resolvers. Signed-off-by: Jonas Jacobsen <[email protected]>

Changeset

cc13343

Signed-off-by: Jonas Jacobsen <[email protected]>

Joklost force-pushed the main branch from e9eef3b to cc13343 Compare November 5, 2022 12:39

Joklost marked this pull request as ready for review November 5, 2022 12:42

Move e2e operation files to appropriate route

b338751

Operation files only used by src/routes/stores/nested-list are now placed appropriately. Signed-off-by: Jonas Jacobsen <[email protected]>

AlecAivazis reviewed Nov 5, 2022

View reviewed changes

AlecAivazis approved these changes Nov 5, 2022

View reviewed changes

AlecAivazis requested changes Nov 5, 2022

View reviewed changes

vercel bot deployed to Preview – docs November 5, 2022 19:40 View deployment

vercel bot deployed to Preview – docs-next November 5, 2022 19:41 View deployment

Joklost force-pushed the main branch from dc85c53 to b338751 Compare November 6, 2022 18:34

vercel bot deployed to Preview – docs November 6, 2022 22:15 View deployment

vercel bot deployed to Preview – docs-next November 6, 2022 22:16 View deployment

Joklost closed this Nov 7, 2022

Joklost reopened this Nov 7, 2022

Joklost closed this Nov 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide parentType to addMany subscriptions #678

Provide parentType to addMany subscriptions #678

Joklost commented Nov 5, 2022 •

edited

Loading

changeset-bot bot commented Nov 5, 2022 •

edited

Loading

vercel bot commented Nov 5, 2022

Joklost commented Nov 5, 2022

jycouet commented Nov 5, 2022

Joklost commented Nov 5, 2022

AlecAivazis Nov 5, 2022

AlecAivazis commented Nov 5, 2022

AlecAivazis Nov 5, 2022

vercel bot commented Nov 5, 2022 •

edited

Loading

AlecAivazis Nov 5, 2022

Joklost Nov 5, 2022

AlecAivazis Nov 5, 2022

Joklost Nov 5, 2022

AlecAivazis Nov 5, 2022

Joklost Nov 6, 2022

AlecAivazis Nov 6, 2022 •

edited

Loading

AlecAivazis commented Nov 7, 2022

Provide parentType to addMany subscriptions #678

Provide parentType to addMany subscriptions #678

Conversation

Joklost commented Nov 5, 2022 • edited Loading

To help everyone out, please make sure your PR does the following:

changeset-bot bot commented Nov 5, 2022 • edited Loading

🦋 Changeset detected

vercel bot commented Nov 5, 2022

Joklost commented Nov 5, 2022

jycouet commented Nov 5, 2022

Joklost commented Nov 5, 2022

Choose a reason for hiding this comment

AlecAivazis commented Nov 5, 2022

Choose a reason for hiding this comment

vercel bot commented Nov 5, 2022 • edited Loading

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

AlecAivazis Nov 6, 2022 • edited Loading

Choose a reason for hiding this comment

AlecAivazis commented Nov 7, 2022

Joklost commented Nov 5, 2022 •

edited

Loading

changeset-bot bot commented Nov 5, 2022 •

edited

Loading

vercel bot commented Nov 5, 2022 •

edited

Loading

AlecAivazis Nov 6, 2022 •

edited

Loading