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

Cleanup async selectors shared across multiple roots #1714

Closed
wants to merge 4 commits into from

Conversation

drarmstr
Copy link
Contributor

@drarmstr drarmstr commented Apr 6, 2022

Summary:
Cleanup the logic used for sharing executions of async selectors shared across multiple Recoil stores.

  • Remove potential memory leak from cached dep values.
  • Fix hanging selectors when reading cached dep values for checking if dep values changed.
  • When checking if an execution from another store is valid, compare the value of the dep for the execution with the current value in the current store instead of the execution's store.
  • Slight optimization if there are many roots or dependencies by using iterables instead of materializing arrays.

Differential Revision: D35426975

Summary:
Pull Request resolved: facebookexperimental#1704

It appears that TypeScript can produce inconsistent ordering for union types across versions while `$ExpectType` requires a consistent ordering.  (microsoft/TypeScript#17944) This can cause our TypeScript tests to break when run across multiple TypeScript versions.

Update the tests to be more robust to work across versions.

Differential Revision: D35266493

fbshipit-source-id: 6fbbe84ebf39794426f6e914145d49ce4fbb0150
…ters (facebookexperimental#1703)

Summary:
Pull Request resolved: facebookexperimental#1703

Fix the typing to explicitly allow `Map<>` and `Set<>`.

Fix the typing for `atomFamily()` and `selectorFamily()` parameters to better allow for objects that are class instances that have a `toJSON()` method to be used, as they are serializable.

Previously it required `toJSON()` to return a `string`, but it could really return any serializable primitive or object.

Previously it used the arrow syntax for the `toJSON()` method which requires that the method is bound to the instance for dereferencing `this` during execution.  That should not be a requirement, though, as we always call it relative to the instance itself.  This change allows custom user classes to be used.  For example:

```
class MyClass {
  _name: string;

  constructor(name: string) {
    this._name = name;
  }

  toJSON(): string {
    return this._name;
  }
}
```

There is no functional change with this diff, only type fixes.

Differential Revision: https://www.internalfb.com/diff/D35233518?entry_point=27

fbshipit-source-id: cb39d3aefc1ebd156761598995c9b4f93ab38fc1
Summary: When there are multiple `<RecoilRoots>` executing the same selector we should check the current store first when looking for an existing execution to use.  This avoids a dev-only error message in React 18 which tests subsequent calls to `getSnapshot()` from `useSyncExternalStore()`.

Differential Revision: https://www.internalfb.com/diff/D35402882?entry_point=27

fbshipit-source-id: 1b9e8c28435e05085ed198a1b12a508f73d49b4c
Summary:
Cleanup the logic used for sharing executions of async selectors shared across multiple Recoil stores.

* Remove potential memory leak from cached dep values.
* Fix hanging selectors when reading cached dep values for checking if dep values changed.
* When checking if an execution from another store is valid, compare the value of the dep for the execution with the current value in the current store instead of the execution's store.
* Slight optimization if there are many roots or dependencies by using iterables instead of materializing arrays.

Differential Revision: D35426975

fbshipit-source-id: 7aef783627ba87ca34178c64306d346628ce4de7
@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported labels Apr 6, 2022
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D35426975

@drarmstr drarmstr self-assigned this Apr 6, 2022
@drarmstr drarmstr added bug Something isn't working performance labels Apr 6, 2022
@drarmstr drarmstr deleted the export-D35426975 branch April 13, 2022 01:59
AlexGuz23 pushed a commit to AlexGuz23/Recoil that referenced this pull request Nov 3, 2022
Summary:
Pull Request resolved: facebookexperimental/Recoil#1714

Cleanup the logic used for sharing executions of async selectors shared across multiple Recoil stores.

* Remove potential memory leak from cached dep values.
* Fix hanging selectors when reading cached dep values for checking if dep values changed.
* When checking if an execution from another store is valid, compare the value of the dep for the execution with the current value in the current store instead of the execution's store.
* Slight optimization if there are many roots or dependencies by using iterables instead of materializing arrays.

Reviewed By: habond

Differential Revision: D35426975

fbshipit-source-id: 6dad498293a8544d953d232e27640bd8a5f1fedb
snipershooter0701 pushed a commit to snipershooter0701/Recoil that referenced this pull request Mar 5, 2023
Summary:
Pull Request resolved: facebookexperimental/Recoil#1714

Cleanup the logic used for sharing executions of async selectors shared across multiple Recoil stores.

* Remove potential memory leak from cached dep values.
* Fix hanging selectors when reading cached dep values for checking if dep values changed.
* When checking if an execution from another store is valid, compare the value of the dep for the execution with the current value in the current store instead of the execution's store.
* Slight optimization if there are many roots or dependencies by using iterables instead of materializing arrays.

Reviewed By: habond

Differential Revision: D35426975

fbshipit-source-id: 6dad498293a8544d953d232e27640bd8a5f1fedb
eagle2722 added a commit to eagle2722/Recoil that referenced this pull request Sep 21, 2024
Summary:
Pull Request resolved: facebookexperimental/Recoil#1714

Cleanup the logic used for sharing executions of async selectors shared across multiple Recoil stores.

* Remove potential memory leak from cached dep values.
* Fix hanging selectors when reading cached dep values for checking if dep values changed.
* When checking if an execution from another store is valid, compare the value of the dep for the execution with the current value in the current store instead of the execution's store.
* Slight optimization if there are many roots or dependencies by using iterables instead of materializing arrays.

Reviewed By: habond

Differential Revision: D35426975

fbshipit-source-id: 6dad498293a8544d953d232e27640bd8a5f1fedb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants