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

Unable to use return in reactive expression #2828

Closed
taylorzane opened this issue May 20, 2019 · 20 comments
Closed

Unable to use return in reactive expression #2828

taylorzane opened this issue May 20, 2019 · 20 comments

Comments

@taylorzane
Copy link
Contributor

When using a return statement in a reactive expression/block an error is thrown:

'return' outside of function (L:C)

Since the reactive expression contents is wrapper in a function at compile time, this shouldn't cause any syntax errors at runtime.

I imagine this will require changes to the svelte compiler and also the eslint plugin rules.

See a simple repro here:

https://svelte.dev/repl/8ca16f5088c34dd89574a064868d9b4d?version=3.4.1

@EmilTholin
Copy link
Member

Hi @taylorzane! You can make the if statement reactive directly if you prefer.

$: if (count <= 10) {
  bar = 'bar' + foo
}

@taylorzane
Copy link
Contributor Author

Hey @EmilTholin, I appreciate the advice. I know I could rewrite the expression as a whole to suit the existing syntax, and perhaps this is a wontfix since it's not "technically" valid JavaScript when uncompiled, but I figured I'd bring it up since the end result would still be valid JS.

@tivac
Copy link
Contributor

tivac commented May 20, 2019

Labels not being treated as a function by the compiler comes back to bite me a lot since I prefer to use early-outs whenever possible.

$: {
    if(!case) {
        return;
    }

    doWork(case);
}

This case can obviously become a simple if/else, but they're not always that simple. The inability to use return or continue or some other form of "stop executing this block" is a fairly frequent annoyance for me in v3.

@taylorzane
Copy link
Contributor Author

👆 That right there is exactly why I'd like support for this. It also keeps nesting at one level lower than using a wrapping if/else.

@EmilTholin
Copy link
Member

Yeah, I can see the appeal, but I would personally rather opt for using an IIFE than start treating blocks as functions.

$: (() => {
  if (count > 10) {
    return
  }

  bar = 'bar' + foo
})();

@tivac
Copy link
Contributor

tivac commented May 20, 2019

I think in my ideal world the svelte compiler would be able to see that pattern and remove it to keep the code streamlined, paying the cost for IIFE every time a reactive statement runs isn't something I'm super comfortable with.

@ryanatkn
Copy link
Contributor

ryanatkn commented May 20, 2019

A possibility would be to have the compiler interpret a bare function following the $: label as an IIFE, because right now it's a no-op. This is somewhere in between the IIFE workaround and the proposed reactive blocks-as-functions.

$: () => {
  // this could be interpreted as an IIFE instead of a no-op: "anonymous reactive function"
}

There's tension between the code you'd ideally write (returns in blocks with compiler smarts) versus code that matches expectations of JS and works better with e.g. TypeScript. I'm curious what Svelte's philosophy is here, what costs will be paid to write more ideal code. Elm is a good example of a language+framework that pays seemingly anything to get to its ideal form.

@Rich-Harris
Copy link
Member

This is a little trickier than just deciding to allow return — we would also need to hack around Acorn so that it accepted a return outside a function (the same could go for break or continue or whatever). It would also get tricky in terms of the generated code — if you add another reactive line...

let count = 0
$: {
  if (count > 10) return;
  bar = 'bar' + foo
}
	
$: somethingElse = bar + 'whee';

...Svelte generates this:

$$self.$$.update = ($$dirty = { count: 1, foo: 1, bar: 1 }) => {
  if ($$dirty.count || $$dirty.foo) { {
    if (count > 10) return;
    $$invalidate('bar', bar = 'bar' + foo)
  } }

  if ($$dirty.bar) { $$invalidate('somethingElse', somethingElse = bar + 'whee'); }
};

In other words it would fail to update somethingElse.

These aren't intractable problems, but it's definitely not something that's straightforward to fix. @tivac's suggestion is the one I like the most — Svelte could generate this from that example:

function $$update_bar() {
  if (count > 10) {
    return
  }

  $$invalidate('bar', bar = 'bar' + foo);
}

$$self.$$.update = ($$dirty = { count: 1, foo: 1, bar: 1 }) => {
  if ($$dirty.count || $$dirty.foo) $$update_bar();
};

@Conduitry Conduitry added awaiting submitter needs a reproduction, or clarification proposal labels May 21, 2019
@thgh
Copy link
Contributor

thgh commented May 25, 2019

It's not valid JavaScript so it would break prettier and other tools. 🤔

@trbrc
Copy link
Contributor

trbrc commented May 27, 2019

Unless the benefit is proven to be very substantial, I don’t think additional creative interpretations of JS semantics will be good for Svelte. I object to it being included on a whim. I’m afraid that this stuff will pile up and lead to unexpected issues in the future.

Using an IIFE as @EmilTholin suggested looks like a good choice. I see at least three solutions to @tivac ‘s performance objection to it:

  • Measure whether it’s even an issue. Is an IIFE expensive in the real world? I don’t know much about JS engines, but unless it’s known to be difficult to optimize, I wouldn’t sweat it.
  • Use a plain function declaration, instead of an IIFE, and call it as a reactive expression passing in the variables that should invalidate it as arguments. ($: myFunc(count, foo);)
  • Make Svelte analyze a reactive declaration with an IIFE and “unwrap it” when possible. (As @Rich-Harris points out, it might be tricky in plenty of common cases though)

@taylorzane
Copy link
Contributor Author

It seems like there's a decent amount of overhead for an IIFE compared to no IIFE (see benchmark). Obviously that's a very naïve test and not indicative of actual usage in a Svelte component. Personally, I have no preference as long as we can get some sort of bail early functionality.

@ryanatkn
Copy link
Contributor

@taylorzane

as long as we can get some sort of bail early functionality

Svelte supports IIFEs already, they just don't look great - https://svelte.dev/repl/99fd078d1bbd46088bcd6fc59e761d4d?version=3.4.4

@tivac
Copy link
Contributor

tivac commented May 27, 2019

Yeah, they do work but they're not handled specially so every time they run they're recreated. 😑

@trbrc
Copy link
Contributor

trbrc commented May 27, 2019

Something just occurred to me that maybe should be obvious.

Svelte is repurposing labeled blocks, by deleting the actual label and moving the block into a function. The only reason someone might expect return to work in this context is because they are looking at the compiled output, which is inside a function.

However, a real labeled block already has a way to exit early. In fact, it's pretty much the only "feature" a real labeled block has:

$: {
	if (count > 10) {
		break $;
	}
	
	bar = 'bar' + foo
}

That's valid JS, but if you put that into the Svelte compiler, it just looks at you funny. It happily accepts the following though:

X: {
	if (count > 10) {
		break X;
	}
	
	bar = 'bar' + foo
}

...but of course doesn't make it reactive.

So, if early exits are even a concern, my suggestion would be to implement it by adding support for labeled breaks. That would bring Svelte semantically closer to "real" JS in its handling of labeled blocks, rather than continue to deviate. It also solves the issue with returns and multiple reactive statements.

I'd even imagine it would be quite straightforward to implement. The compiler would simply not delete the label when it moves the reactive declaration into the update function.

Example change:

	$$self.$$.update = ($$dirty = { count: 1, foo: 1 }) => {
-		if ($$dirty.count || $$dirty.foo) { {
+		if ($$dirty.count || $$dirty.foo) { $: {
				if (count > 10) {
					break $;
				}
				
				$$invalidate('bar', bar = 'bar' + foo)
			} }
	};

@theSherwood
Copy link

theSherwood commented May 28, 2019

This isn't pretty, but a workaround that you can do at the moment is to put a label statement in the reactive statement. It's less code than an iife anyway. The REPL will let you get away with the following:

$: X: if (count >= 10) { alert('count is dangerously high!'); break X; count = 9; }

This always bails out early. The count never gets set back to nine.

@theSherwood
Copy link

theSherwood commented May 28, 2019

Hilariously, the above hack will also likely be backwards compatible if @trbrc 's solution is adopted because labels can be chained together. The following also works:

$: X: Y: if (count >= 10) { alert('count is dangerously high!'); break X; count = 9; break Y; }

@Conduitry Conduitry removed the awaiting submitter needs a reproduction, or clarification label Oct 12, 2019
@Conduitry
Copy link
Member

(Unless it's changed again!) this will be resolved by #3539 by letting you break $ inside reactive blocks.

@Conduitry
Copy link
Member

It turns out that this is resolved for browser builds by #3539, but it's not yet working for SSR builds. The $: label isn't getting preserved in that case.

@ITenthusiasm
Copy link

Do we have anything on the Tutorials referencing this new feature? Would that be useful to have?

@basaran
Copy link

basaran commented Nov 5, 2021

I too would find it useful to be able to do early returns or breaking out of the labeled reactive blocks. It helps with not nesting the code too much.

Benchmark link from @taylorzane is broken, could anyone please clarify if a dummy function such as:

$: things(reactiveVar);

has different or same performance as:

$: if (reactiveVar) {
   
}

or would there be any consequences?

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

No branches or pull requests