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

Singleton useObject #4544

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

Conversation

mfbx9da4
Copy link

@mfbx9da4 mfbx9da4 commented May 2, 2022

What, How & Why?

To play nicely with React's immutability conventions this PR introduces a singleton object which is returned by useObject.

This closes #4521

⚠️ For the time being this is untested illustrative code. Opened as a draft PR to open discussion with @takameyer.

☑️ ToDos

  • Return singleton object
  • One realm listener per singleton object
  • Handle deleted or invalid objects gracefully
  • Handle objects not yet inserted gracefully
  • Support type being a constructor
  • Make all existing tests pass
  • Test listening to object before it is inserted
  • Test listening to object after it is deleted and then inserted again
  • Test there are no memory leaks after
  • Test referential equality
  • Do the same for collections
    ...
  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 🚦 Tests
  • 📱 Check the React Native/other sample apps work if necessary
  • 📝 Public documentation PR created or is not necessary
  • 💥 Breaking label has been applied or is not necessary

If this PR adds or changes public API's:

  • typescript definitions file is updated
  • jsdoc files updated
  • Chrome debug API is updated if API is available on React Native

@cla-bot cla-bot bot added the cla: yes label May 2, 2022
@mfbx9da4 mfbx9da4 marked this pull request as draft May 2, 2022 17:26
@takameyer
Copy link
Contributor

@mfbx9da4 Thanks for submitting this. I'll look it over with the team. Next week I may have some time to add some tests to verify this.

@mfbx9da4
Copy link
Author

mfbx9da4 commented May 4, 2022

Awesome, thanks. I have a couple other todos pending including handling deleted objects gracefully. I'll let you know if I get some availability to work on this further.

@mfbx9da4
Copy link
Author

mfbx9da4 commented May 5, 2022

Had a little more time to work on it this evening. Again, as yet untested, though I did manage to follow your instructions and run the tests in general. Overall, I think it's shaping up quite nicely though! 🙂

@mfbx9da4
Copy link
Author

mfbx9da4 commented May 6, 2022

Update @takameyer, managed to run the tests and add a test. I've got a few failing ones which I'm a bit confused about since I didn't really modify the way it works. Specifically does not re-render if an invisible list item is modified without a UI interaction, perhaps you have some insight?

I'm sure I'll be able to work it out eventually but if you have a moment 🙂

@takameyer
Copy link
Contributor

Ill have more time to review this next week. Really appreciate the effort here!

@takameyer
Copy link
Contributor

@mfbx9da4 We have been discussing internally how we want to generally handle external PRs. I'll first quote an upcoming update to our contribution guide:

[...] indicate how you would like us to support you. We will happily guide you and work with you to move the PR to a point where it can be merged. It might require considerable work at your end to meet our expectations (code quality, API docs, TS defs, tests, etc.). In the case you want to move on and not work with us, please let us know. If the PR meets our expectations, we will merge it - and if it doesn't, we will either take over or close it, depending on the requirement on our time and our current priorities.

So, let us know your intentions with the PR 🙂. We have this on our backlog, but it will probably take a while before this gets prioritised on our end. If you would like to work on it, it may take some time with the back and forth and a bit of patience, but will in the end be more likely to be accepted into our code base.

@takameyer
Copy link
Contributor

@mfbx9da4 Sorry for the delay. I've added @tomduncalf and @kraenhansen to the PR as well. Let's work together to get this in.

@@ -84,11 +84,23 @@ describe("useObject hook", () => {
expect(object).toMatchObject(dog2);
});

it("object is null", () => {
it("object is undefined", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

symantically this should be null if the object doesn't exist

const object2 = result2.current;

expect(object).toBeDefined();
expect(object).toEqual(object2);
Copy link
Contributor

Choose a reason for hiding this comment

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

This passes no matter if the reference is different. Change to toBe

@TommyTheTribe
Copy link

Does anyone know why this branch has not been merged ? Thanks

@kraenhansen
Copy link
Member

@TommyTheTribe looks stale to me. I see @takameyer made a few comments which was not addressed and now it also needs a rebase.

@takameyer
Copy link
Contributor

@TommyTheTribe I added tests for the changes made here and they failed. We haven't had the resources available to tackle this ourselves just yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

useObject will return referentially different objects for the same object
4 participants