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

[docs] Update tutorial on component binding #6852

Closed

Conversation

RandomComm3nt
Copy link

Updated sample code to remove boilerplate code and removed note warning about an error that is no longer present. It seems like previously it was not possible to directly reference a function on a child component, but this is no longer the case.

Discussed this in the discourse and someone said this error was fixed in 3.17 - at a guess, by this issue: #4166 (but I'm new to Svelte so could be completely wrong about that)

Updated sample code to remove boilerplate code and removed note warning about an error that is no longer present
@ignatiusmb
Copy link
Member

Interesting, in that case I'm guessing we should also update https://svelte.dev/docs#bind_component as well?

> Note that we can't do `{cart.empty}` since `cart` is `undefined` when the button is first rendered and throws an error.

@ignatiusmb ignatiusmb changed the title [fix] Update tutorial on component binding [docs] Update tutorial on component binding Oct 18, 2021
Removed note about an error that no longer exists and removed boilerplate code from an example
@RandomComm3nt
Copy link
Author

@ignatiusmb I've added the matching change to the docs

@Conduitry
Copy link
Member

I'd definitely want to bisect first to figure out when that changed and see whether it was intentional and backed by a test before changing this, because this sounds like something that could easily change back while changing something else if it's not specifying tested for.

@RandomComm3nt
Copy link
Author

RandomComm3nt commented Oct 18, 2021

This was fixed in 3.16.7:
https://svelte.dev/repl/f5adf0a038fc4e69b95c6cfeacdab31c?version=3.16.6 (errors)
https://svelte.dev/repl/f5adf0a038fc4e69b95c6cfeacdab31c?version=3.17.7 (works)

I've added a test that passes on the latest code but fails on 3.16.6 (tested on commit f805a2c).
I am pretty sure this was fixed in this PR: #4155 and is covered by a test in that commit.

With regards to whether this is intentional, in the 3.16.6 repl above, if you swap lines 8 and 9 it runs fine. That is, before this was changed:
<InputField bind:this={field}/> <button on:click={field.focus}>Focus field</button>
worked but
<button on:click={field.focus}>Focus field</button> <InputField bind:this={field}/>
gave an error. I wouldn't expect this order-dependent behaviour.

@bluwy
Copy link
Member

bluwy commented Oct 18, 2021

I'm not sure about this change, it looks like it's possible now because on:click gets compiled into an anonymous function that calls field.focus(), not because field is available on the initial render (it's still not). If you try to place {typeof field.focus} anywhere in the markup, you'll notice that it'll cause an error. And so I think we should keep the code we have now.

@RandomComm3nt
Copy link
Author

@bluwy This is specifically referring to binding, so I'm not sure that it's relevant if it breaks by putting it directly in the markup. The current recommendation of the docs also gives an error if inserted directly into the markup:

<script>
	import InputField from './InputField.svelte';
	let field;
	let a = () => field.focus;
</script>

{a()} <!-- errors -->
<InputField bind:this={field}/>

@bluwy
Copy link
Member

bluwy commented Oct 18, 2021

Even if it works with this PR's change, we don't want to lead users to the mindset that field can be used on the initial render. This won't always work and the user would be confused otherwise. If the user happens to test out placing {typeof field.focus} in the markup, they should know the caveat upfront. And if so, it would be confusing to show this PR's change as they would conflict.

@RandomComm3nt
Copy link
Author

The first reason for adopting Svelte given on the homepage is "write less code", so the tutorial shouldn't be encouraging writing boilerplate code. If the change in my PR doesn't capture the nuance of the language or something then we can amend it to be clearer to new users, but as it stands the tutorial and documentation explicitly state something that is not true.

@bluwy
Copy link
Member

bluwy commented Oct 25, 2021

I dont think an extra () => is boilerplate. IMO for this case, it's not about writing less code, but the mindset of how bind works. What this PR does now is exploiting an implementation detail of the compiler and isn't something I'd encouraged doing. With that, what the documentation explains still holds true besides using it as event handlers, so I don't think there's a need to change here.

@tanhauhau
Copy link
Member

The statement

Note that we can't do {cart.empty} since cart is undefined when the button is first rendered and throws an error.

is not wrong.

using it within event handler happens to just work, because (implementation detail that could change anytime) we evaluate it lazily to allow dynamically get the latest cart instance.

however, this do not work in other scenarios, such as:

<ShoppingCart bind:this={cart}/>

<!-- Error: Cannot read properties of undefined -->
<Foo empty={cart.empty} />

<!-- Error: Cannot read properties of undefined -->
<div>{cart.empty}</div>

<!-- Error: Cannot read properties of undefined -->
{#if cart.empty}
   <div />
{/if}

@tanhauhau tanhauhau closed this Apr 16, 2022
@bluwy bluwy mentioned this pull request May 16, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants