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

Observable-as-store error on reassignment #3304

Closed
kylecordes opened this issue Jul 28, 2019 · 4 comments · Fixed by #3315
Closed

Observable-as-store error on reassignment #3304

kylecordes opened this issue Jul 28, 2019 · 4 comments · Fixed by #3315
Assignees
Labels

Comments

@kylecordes
Copy link

kylecordes commented Jul 28, 2019

Describe the bug

Reassigning an observable-as-store variable results in an error $$unsubscribe_NAMEHERE is not a function. It appears that currently observables can be used only in certain undocumented restricted ways, perhaps only as long as such variables are never reassigned?

To Reproduce

https://svelte.dev/repl/c73aa3a571844702baa8538c80376603?version=3.6.9

<script>
  import { of } from 'rxjs';

  let myObs = of([1, 2, 3, 4, 5]);
  myObs = of([6, 7, 8, 9, 10]);
</script>

{$myObs}

Expected behavior

Runs without error.

Severity

It's not severe at all if you never use RxJS, but it's a pretty significant restriction if you do (!).

Fixing this seems necessary to declare reasonably observable-as-store support.

@Conduitry Conduitry added the bug label Jul 28, 2019
@Conduitry
Copy link
Member

Here when we're setting the unsubscription function to be called later, we're not accounting for the fact that the return value of .subscribe() might be an object with a .unsubscribe() method, rather than an unsubscription function directly. We can either put a check for that inline here (which would be generated for each reassigned store in each component), or we can extract that logic into its own internal helper. The logic we need is already in the subscribe internal helper, but it's along with some other component-specific stuff we don't want. I'm thinking we should factor some sort of 'just subscribe, and return a normalized unsubscription function' helper out of subscribe and get_store_value, and use that here as well.

@Conduitry Conduitry self-assigned this Jul 30, 2019
@Conduitry
Copy link
Member

Current WIP implementation:

diff --git a/src/compiler/compile/render_dom/index.ts b/src/compiler/compile/render_dom/index.ts
index c6eb1f1b..df5d1526 100644
--- a/src/compiler/compile/render_dom/index.ts
+++ b/src/compiler/compile/render_dom/index.ts
@@ -390,7 +390,7 @@ export default function dom(
 
 			const store = component.var_lookup.get(name);
 			if (store && store.reassigned) {
-				return `${$name}, $$unsubscribe_${name} = @noop, $$subscribe_${name} = () => { $$unsubscribe_${name}(); $$unsubscribe_${name} = ${name}.subscribe($$value => { ${$name} = $$value; $$invalidate('${$name}', ${$name}); }) }`;
+				return `${$name}, $$unsubscribe_${name} = @noop, $$subscribe_${name} = () => { $$unsubscribe_${name}(); $$unsubscribe_${name} = @just_subscribe(${name},$$value => { ${$name} = $$value; $$invalidate('${$name}', ${$name}); }) }`;
 			}
 
 			return $name;
diff --git a/src/runtime/internal/ssr.ts b/src/runtime/internal/ssr.ts
index 1ae1ae1d..d84efc73 100644
--- a/src/runtime/internal/ssr.ts
+++ b/src/runtime/internal/ssr.ts
@@ -1,6 +1,5 @@
 import { set_current_component, current_component } from './lifecycle';
 import { run_all, blank_object } from './utils';
-import { Readable } from 'svelte/store';
 
 export const invalid_attribute_name_character = /[\s'">/=\u{FDD0}-\u{FDEF}\u{FFFE}\u{FFFF}\u{1FFFE}\u{1FFFF}\u{2FFFE}\u{2FFFF}\u{3FFFE}\u{3FFFF}\u{4FFFE}\u{4FFFF}\u{5FFFE}\u{5FFFF}\u{6FFFE}\u{6FFFF}\u{7FFFE}\u{7FFFF}\u{8FFFE}\u{8FFFF}\u{9FFFE}\u{9FFFF}\u{AFFFE}\u{AFFFF}\u{BFFFE}\u{BFFFF}\u{CFFFE}\u{CFFFF}\u{DFFFE}\u{DFFFF}\u{EFFFE}\u{EFFFF}\u{FFFFE}\u{FFFFF}\u{10FFFE}\u{10FFFF}]/u;
 // https://html.spec.whatwg.org/multipage/syntax.html#attributes-2
@@ -121,18 +120,6 @@ export function create_ssr_component(fn) {
 	};
 }
 
-/**
- * Get the current value from a store by subscribing and immediately unsubscribing.
- * @param store readable
- */
-export function get_store_value<T>(store: Readable<T>): T | undefined {
-	let value;
-	const unsubscribe: any = store.subscribe(_ => value = _);
-	if (unsubscribe.unsubscribe) unsubscribe.unsubscribe();
-	else unsubscribe();
-	return value;
-}
-
 export function add_attribute(name, value) {
 	if (!value) return '';
 	return ` ${name}${value === true ? '' : `=${typeof value === 'string' ? JSON.stringify(value) : `"${value}"`}`}`;
diff --git a/src/runtime/internal/utils.ts b/src/runtime/internal/utils.ts
index 08410ec3..a5c8cff2 100644
--- a/src/runtime/internal/utils.ts
+++ b/src/runtime/internal/utils.ts
@@ -48,12 +48,19 @@ export function validate_store(store, name) {
 	}
 }
 
-export function subscribe(component, store, callback) {
+export function just_subscribe(store, callback) {
 	const unsub = store.subscribe(callback);
+	return unsub.unsubscribe ? () => unsub.unsubscribe() : unsub;
+}
 
-	component.$$.on_destroy.push(unsub.unsubscribe
-		? () => unsub.unsubscribe()
-		: unsub);
+export function get_store_value(store) {
+	let value;
+	just_subscribe(store, _ => value = _)();
+	return value;
+}
+
+export function subscribe(component, store, callback) {
+	component.$$.on_destroy.push(just_subscribe(store, callback));
 }
 
 export function create_slot(definition, ctx, fn) {

For some reason, I was getting all sorts of bizarre TypeScript errors on unrelated parts of the code when I moved get_store_value from ssr.ts to utils.ts. Removing its type definitions helped, but we're going to want to add those back in again probably.

@Conduitry
Copy link
Member

Different direction of attack. I think this is doing the correct thing:

/**
 * Get the current value from a store by subscribing and immediately unsubscribing.
 * @param store readable
 */
export const get = get_store_value as {
	<T>(store: Readable<T>): T;
};

This is what I'm doing now in store.ts instead of a regular re-export of get_store_value.

Conduitry added a commit to Conduitry/sveltejs_svelte that referenced this issue Jul 30, 2019
Rich-Harris added a commit that referenced this issue Aug 3, 2019
Support RxJS Observables in reassigned stores
@Conduitry
Copy link
Member

This is fixed in 3.6.11 - https://svelte.dev/repl/c73aa3a571844702baa8538c80376603?version=3.6.11

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 a pull request may close this issue.

2 participants