Skip to content

[WIP] Implement reactive assignments #1839

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

Merged
merged 269 commits into from
Dec 16, 2018
Merged

[WIP] Implement reactive assignments #1839

merged 269 commits into from
Dec 16, 2018

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Nov 5, 2018

This is gonna take a while. First order of business is updating all the tests. I'm going to see how far I can get doing it via svelte-upgrade: sveltejs/svelte-upgrade#12


Assignment instrumentation:

  • (Nice-to-have) Short-circuit boolean operations have $$make_dirty called outside of the boolean operation:
<script>
	let foo;
	let bar;
	$: foo && (bar = 1); // is made dirty outside `&&`
</script>

<script>
export default {
onrender () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I stared at this for a while going "wtf is onrender". It wasn't until I went and did a search that I realized it's deprecated syntax.

Glad this is getting updated, feels weird that the tests were using it 🤔

@Rich-Harris
Copy link
Member Author

Ok, the tests are done-ish — I clumsily squashed and force-pushed the couple dozen or so individual commits. (Some of them no longer really make sense, some will need to be renamed, some test things that we need to find alternatives for, like transition events.)

A lot of the test JavaScript (as opposed to the components) will no longer work, since it uses component.set and other things that no longer exist. But this makes it possible to start implementing reactive assignments.

@@ -2,18 +2,16 @@
"name": "svelte",
"version": "2.15.1",
"description": "The magical disappearing UI framework",
"main": "compiler/svelte.js",
"main": "index.js",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should index.js (the lifecycle etc hooks functions) be pkg.module rather than pkg.main? They're not in commonjs, nor do I think they should be.

That would leave the question of 'well what should pkg.main be?' I don't know. Maybe nothing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. Maybe nothing? Or maybe we capitulate to Node and have

"main": "index.js",
"module": "index.mjs"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about pkg.main points to a file that just throws a big old exception and lets the user know what and how they should be importing instead? (Hopefully there aren't any current bundlers that will use pkg.main when pkg.module is available.) The message could say 'lifecycle stuff lives here, the compiler lives here, animation stuff lives here, observable stuff lives here' etc

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've adopted putting "main": "index" (without the extension) — as far as I know, according to the current Node modules draft, it will choose the appropriate extension depending on whether it is imported or required.

@Conduitry
Copy link
Member

Conduitry commented Dec 12, 2018

Is there a real reason to keep the <svelte:component> syntax around anymore in v3? I don't think it can do anything that <Foo/> using an instance-scoped variable Foo can't do, and it seems to break the guideline of Don't have two ways to do the same thing.

@Rich-Harris
Copy link
Member Author

I did wonder about that. I think there probably are cases where it's still useful. For example in Sapper a layout component needs to do this...

<svelte:component this={child.component} {...child.props}/>

...so to achieve the same thing we'd need to do one of these:

<!-- lowercased name implies DOM node, and member expressions
     aren't currently supported -->
<child.component {...child.props}/>
<script>
  export let child;

  let Component;
  $: Component = child.component;
</script>

<Component {...props}/>

(Aside: it suddenly occurs to me that layout components will need to explicitly declare the child prop in v3.)

There's also a cost to assuming that a component's constructor could change — Svelte needs to generate more code for <svelte:component this={Foo}/> than for <Foo/>. In most cases it would be able to determine whether a constructor could change (i.e. const is unchangeable, and import is assumed to be, as could let if there were no reassignments), but that's not true in the case of props. There's value in being able to differentiate between these:

<!-- a virtual scroll component, maybe -->
<script>
  export let items;
  export let Row;
</script>

{#each items.slice(start, end) as item}
  <Row {...item}/>
{/each}
<script>
  export let items;
  export let Row;
</script>

{#each items.slice(start, end) as item}
  <svelte:component this={Row} {...item}/>
{/each}

So maybe the way to look at this is that it doesn't break the guideline as they're two separate problems — if you want a component to be changeable, use <svelte:component>, otherwise use the constructor directly.

@lukeed
Copy link
Member

lukeed commented Dec 12, 2018

I'm still using svelte:component because (last I checked) Foo will throw an error if not set with initial Component value.

But, in the event of Foo being a page to render (determined via URL), that Foo arguably shouldn't have (or shouldn't need) a default component .

See here for example

@Rich-Harris Rich-Harris merged commit f45e2b7 into master Dec 16, 2018
@Rich-Harris Rich-Harris deleted the rfc-1 branch December 16, 2018 00:18
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

Successfully merging this pull request may close these issues.

6 participants