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

adds reconcileWithKeys #2280

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

titoBouzout
Copy link
Contributor

@titoBouzout titoBouzout commented Sep 12, 2024

Summary

A suggestion to add reconcileWithKeys that @katywings and I have requested in the past. In some situations you want to use reconcile with objects that have keys differently named to id.

       const [target, setStore] = createStore({
        c: {
          a: [
            { ida: 1, name: "1" },
            { ida: 0, name: "0" }
          ],
          b: [
            { idb: 1, name: "1" },
            { idb: 0, name: "0" }
          ]
        }
      });

      const ref = target.c.a[1];

      const source = {
        c: {
          a: [{ ida: 0, name: "0 modified" }],
          b: [{ idb: 0, name: "0 modified" }]
        }
      };

      /*
      setStore("c", reconcile(source.c));
      expect(target.c.a[0]).not.toBe(ref);
      */

      setStore(
        "c",
        reconcileWithKeys(source.c, {
          keys: {
            a: { _key: "ida" },
            b: { _key: "idb" }
          }
        })
      );
      expect(target.c.a[0]).toBe(ref);

      expect(target).toEqual({
        c: {
          a: [{ ida: 0, name: "0 modified" }],
          b: [{ idb: 0, name: "0 modified" }]
        }
      });

How did you test this change?

Hehe, Im running into an issue when trying to write tests for it, when I run npm run test I get this error:

solid-js:test:  FAIL  store/test/modifiers.spec.ts > reconcileWithKeys
solid-js:test: TypeError: Cannot assign to read only property 'Symbol(solid-proxy)' of object '#<Object>'
solid-js:test: ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯

It works in the playground, so I'm lost at to whats going on, any ideas? 🙏🏼
https://playground.solidjs.com/anonymous/9167e413-e0b2-45bf-942a-3ca2153c0416

References

https://discord.com/channels/722131463138705510/780502110772658196/971305532034011156
https://discord.com/channels/722131463138705510/780502110772658196/1278247899616247808

Copy link

changeset-bot bot commented Sep 12, 2024

⚠️ No Changeset found

Latest commit: fc2d65a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@titoBouzout
Copy link
Contributor Author

Ah, I see what's going on, it's failing a test (as expected), but then its throwing an unrelated error, that put me off, and I wasn't understanding. I think I can continue writing tests..

@ryansolid
Copy link
Member

It's a cool Idea. There have been a few design variations on this. Not completely sure which makes the most sense. Mostly wonder if the changes to Stores in 2.0 will have bigger bearing on what the design ultimately is.

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.

2 participants