fix(utils): Initial data for atomWithObservable #1058

11bit · 2022-03-12T11:14:18Z

Reimplementation of initial data option for atomWIthObservable inspired by #875 + some refactoring and simplification of atomWithObservable code

vercel · 2022-03-12T11:14:23Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pmndrs/jotai/6rmNcNquLaDujuuH2drTj4BF9KN9
✅ Preview: https://jotai-git-fork-11bit-feature-observable-initial-data-pmndrs.vercel.app

codesandbox-ci · 2022-03-12T11:15:22Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a55f7e3:

Sandbox	Source
React	Configuration
React Typescript	Configuration
React Browserify	Configuration
React Snowpack	Configuration
Next.js	Configuration
Next.js with custom Babel config	Configuration
React with custom Babel config	Configuration

dai-shi

Hey, nice to see you work on this!! Please see comments.

dai-shi · 2022-03-14T04:50:46Z

src/utils/atomWithObservable.ts

@@ -29,69 +29,52 @@ type ObservableLike<T> = {

 type SubjectLike<T> = ObservableLike<T> & Observer<T>

+type InitialDataFunction<T> = () => T | undefined
+
+export type AtomWithObservableOptions<TData> = {


I know it's a bit controversial, but we prefer not exporting types.

Suggested change

export type AtomWithObservableOptions<TData> = {

type AtomWithObservableOptions<TData> = {

dai-shi · 2022-03-14T04:51:27Z

src/utils/atomWithObservable.ts

+export type CreateObservableOptions<TData> = {
+  initialData?: TData
+}


I don't think we need this. Leftover?

Suggested change

export type CreateObservableOptions<TData> = {

initialData?: TData

}

yes, accidentally copied that from the previous pr. Thanks for noticing

dai-shi · 2022-03-14T04:51:53Z

src/utils/atomWithObservable.ts

+  initialData?: TData | InitialDataFunction<TData>
+}
+export type CreateObservableOptions<TData> = {
+  initialData?: TData


Can we name it initialValue?

Suggested change

initialData?: TData

initialValue?: TData

Renamed everywhere to initialValue

dai-shi · 2022-03-14T04:58:47Z

src/utils/atomWithObservable.ts

+function firstValueFrom<T>(source: ObservableLike<T>): Promise<T> {
+  return new Promise<T>((resolve, reject) => {
+    let resolved = false
+    const subscription = source.subscribe({


Although this looks cleaner, I think there's a risk of memory leaks.
Such case is, a) we create an atom, b) before getting the first value, c) atom can be unmounted.
We need to unsubscribe this subscription on onMount cleanup.

@dai-shi hmm, I thought that the suspended component is not mounted before the promise resolves. So, we don't actually have an option to unsubscribe in any case or do I miss something?

To illustrate, here is an atom that suspends and onMount is never called https://codesandbox.io/s/modern-glade-rutmog?file=/src/App.js:0-413

Yeah, you are right about it. The risk is actually the case onMount is never called. And, it's not covered with the previous impl either...

In jotai/urql code, we have a hacky solution with setTimeout.

jotai/src/urql/atomWithSubscription.ts

Lines 86 to 89 in 2c751b0

let timer: NodeJS.Timeout | null = setTimeout(() => {

timer = null

subscriptionInRender.unsubscribe()

}, 1000)

Given that this isn't ideal, and this is such a rare case anyway. I think I can merge this PR. Maybe, we leave some comments for the future.

BTW I've already changed other things. Should I do anything else here?

Let's add a comment like this:

// FIXME this implementation is not fully compatible with concurrent rendering. // we need to deal with the case `onMount` is not invoked after the atom is initialized.

@dai-shi I've added the comment. Please have a look

- Remove leftovers - Rename initialData => initialValue

…le with concurrent rendering

src/utils/atomWithObservable.ts

dai-shi

LGTM
Thanks for your contribution!

dai-shi · 2022-03-31T09:34:07Z

#1058 (comment)
After reading my comment again, I notice I misunderstood something.
I think I confused with several issues.

Issue 1: If we subscribe in render (= read function body), there might not be a way to unsubscribe, leading to memory leaks.
Issue 2: Even if we unsubscribe in the callback, it may not fire.
Issue 3: Ideally, we should just subscribe once, if it mounts soon.

Issue 1 is important, but we unsubscribe in the callback (both before this PR and this PR), so it's not a big issue if the callback is invoked soon.
But, Issue 2 is still present for both. We would like to fix it but the known workaround is only the setTimeout hack done in jotai/query (internally in RQ) and jotai/urql.
Issue 3 is something introduced in this PR. It's only the performance issue, and if subscribe is lightweight, it's not a big issue.

So, if Issue 3 is something we care, we would like to revert the change. What do you think? @11bit

The FIXME comment should be about Issue 2. Let me try clarifying more.

dai-shi

Wait, there is more important issue.
Issue 4: If the value is changed twice the second value is dropped.
I guess it's the regression with this PR.

So, let's revert it, or come up with a better solution.

dai-shi · 2022-03-31T09:47:47Z

Issue 4: If the value is changed twice the second value is dropped.

Hmm, it's not solved even with the original code without this PR.
My bad again. I'll wait for comments anyway, hoping we have a better solution.

dai-shi

The biggest concern is Issue 4, but it's not a regression of this PR.
Issue 3 is the one introduced in this PR, but only a performance concern.

So, at least, it's mergeable.

11bit · 2022-03-31T11:30:55Z

So, you mean that current implementation can miss values received between first value and befoe onMount, right?

I think solution might be to subscribe only once and do the following:

First value => resolve promise
Store everything that is receiverd before onMount somewhere
onMount => set the last received value from step 2 and then work as usual

dai-shi · 2022-03-31T12:03:12Z

So, you mean that current implementation can miss values received between first value and befoe onMount, right?

Yes. The previous implementation before this PR can miss them too, but it throws an error in such case. (So, it's sort of a regression?)

2. Store everything that is receiverd before onMount somewhere

"somewhere" will be a variable in the scope, which is fine.
But, then we will have Issue 1. An atom can be initialized and never be mounted.

Ivan Buryak added 2 commits March 12, 2022 14:16

Add an option to set initial data for atomWithObservable

3c93b7c

Simplify atomWithObservable code

0182706

vercel bot deployed to Preview March 12, 2022 11:14 View deployment

11bit mentioned this pull request Mar 12, 2022

fix(utils): atomWithObservable is not resubscribed after remount #1016

Merged

Update docs

14aca92

vercel bot deployed to Preview March 13, 2022 06:39 View deployment

11bit marked this pull request as ready for review March 13, 2022 06:47

dai-shi requested changes Mar 14, 2022

View reviewed changes

- Don't export types

1ea4c7b

- Remove leftovers - Rename initialData => initialValue

vercel bot deployed to Preview March 14, 2022 10:31 View deployment

Add a fixme that current subscription mechanics is not fully compatib…

4eb0d3b

…le with concurrent rendering

vercel bot deployed to Preview March 29, 2022 06:25 View deployment

dai-shi reviewed Mar 29, 2022

View reviewed changes

src/utils/atomWithObservable.ts Outdated Show resolved Hide resolved

dai-shi approved these changes Mar 29, 2022

View reviewed changes

Update src/utils/atomWithObservable.ts

5fa37fd

vercel bot deployed to Preview March 29, 2022 08:29 View deployment

dai-shi added this to the v1.6.2 milestone Mar 29, 2022

dai-shi mentioned this pull request Mar 29, 2022

atomWithObservable options to allow enabled and initialData #875

Closed

clarify comment hopefully

e8f976b

vercel bot deployed to Preview March 31, 2022 09:43 View deployment

dai-shi requested changes Mar 31, 2022

View reviewed changes

dai-shi approved these changes Mar 31, 2022

View reviewed changes

dai-shi changed the title ~~Initial data for atomWithObservable~~ fix(utils): Initial data for atomWithObservable Mar 31, 2022

Merge branch 'main' into feature/observable-initial-data

2686800

more comments

a55f7e3

vercel bot deployed to Preview April 4, 2022 23:09 View deployment

dai-shi merged commit 4d479f2 into pmndrs:main Apr 4, 2022

tobias-walle mentioned this pull request May 17, 2022

fix(utils): fix memory leaks and other issues in atomWithObservable #1170

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(utils): Initial data for atomWithObservable #1058

fix(utils): Initial data for atomWithObservable #1058

11bit commented Mar 12, 2022

vercel bot commented Mar 12, 2022 •

edited

Loading

codesandbox-ci bot commented Mar 12, 2022 •

edited

Loading

dai-shi left a comment

dai-shi Mar 14, 2022

11bit Mar 14, 2022

dai-shi Mar 14, 2022

11bit Mar 14, 2022

dai-shi Mar 14, 2022

11bit Mar 14, 2022

dai-shi Mar 14, 2022

11bit Mar 14, 2022

dai-shi Mar 14, 2022

11bit Mar 24, 2022

dai-shi Mar 25, 2022

11bit Mar 29, 2022

dai-shi left a comment

dai-shi commented Mar 31, 2022

dai-shi left a comment

dai-shi commented Mar 31, 2022

dai-shi left a comment

11bit commented Mar 31, 2022

dai-shi commented Mar 31, 2022

	export type AtomWithObservableOptions<TData> = {
	type AtomWithObservableOptions<TData> = {

	export type CreateObservableOptions<TData> = {
	initialData?: TData
	}

	let timer: NodeJS.Timeout \| null = setTimeout(() => {
	timer = null
	subscriptionInRender.unsubscribe()
	}, 1000)

fix(utils): Initial data for atomWithObservable #1058

fix(utils): Initial data for atomWithObservable #1058

Conversation

11bit commented Mar 12, 2022

vercel bot commented Mar 12, 2022 • edited Loading

codesandbox-ci bot commented Mar 12, 2022 • edited Loading

dai-shi left a 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

dai-shi left a comment

Choose a reason for hiding this comment

dai-shi commented Mar 31, 2022

dai-shi left a comment

Choose a reason for hiding this comment

dai-shi commented Mar 31, 2022

dai-shi left a comment

Choose a reason for hiding this comment

11bit commented Mar 31, 2022

dai-shi commented Mar 31, 2022

vercel bot commented Mar 12, 2022 •

edited

Loading

codesandbox-ci bot commented Mar 12, 2022 •

edited

Loading