Deprecate this package for Svelte v3?

klaussner commented 5 years ago

The API of this package doesn't work anymore with Svelte v3. Luckily, using Tracker with the new version much easier, so we might not need svelte-tracker anymore. Here's a small example:

import { Tracker } from "meteor/tracker";
import { onDestroy } from "svelte";

const computation = Tracker.autorun(() => {
  // Use a reactive data source ...
});

onDestroy(() => {
  computation.stop();
});

The only thing that could be simplified/abstracted even more is the onDestroy lifecycle function:

// useTracker.js

export default function useTracker(fn) {
  const computation = Tracker.autorun(() => fn());
  onDestroy(() => computation.stop());
}

import { Tracker } from "meteor/tracker";
import useTracker from "./useTracker.js";

useTracker(() => {
  // Use a reactive data source...
});

But I'm not sure if the tiny useTracker function is worth its own package. 🤔

zimme commented 5 years ago

Sounds reasonable 👍

CaptainN commented 5 years ago

I wonder if there is a way to set up custom bindings, to add that useTracker boilerplate at compile time? (I'm brand new to Svelte, but that would be sweet!)

I'm thinking something like:

t$: myThings = MyCollection.find({ ... stuff ... }).fetch();

would get turned into:

var myThings = useTracker(() => MyCollection.find({ ... stuff ... }).fetch(); );

CaptainN commented 5 years ago

I ended up with something slightly more complex to support SSR:

/* global Meteor */
import { Tracker } from 'meteor/tracker'
import { onDestroy } from 'svelte'

function trackClient (func) {
  // Use Tracker.nonreactive in case we are inside a Tracker Computation.
  // This can happen if someone starts rendering inside a Computation.
  // In that case, we want to opt out of the normal behavior of nested
  // Computations, where if the outer one is invalidated or stopped,
  // it stops the inner one.
  const computation = Tracker.nonreactive(() =>
    Tracker.autorun(func)
  )
  onDestroy(() => {
    computation.stop()
  })
}
function trackServer (func) {
  return func()
}

export const track = Meteor.isClient
  ? trackClient
  : trackServer

Complex enough to warrant a package?

In meteor package this would of course be simpler - one file for each linked directly to server/client.

klaussner commented 5 years ago

Thanks, that looks good! I haven't thought of SSR when I opened this issue.

CaptainN commented 5 years ago

I think trackServer needs a Tracker.nonreactive wrapper - here's the same updated:

/* global Meteor */
import { Tracker } from 'meteor/tracker'
import { onDestroy } from 'svelte'

function trackClient (func) {
  // Use Tracker.nonreactive in case we are inside a Tracker Computation.
  // This can happen if someone starts rendering inside a Computation.
  // In that case, we want to opt out of the normal behavior of nested
  // Computations, where if the outer one is invalidated or stopped,
  // it stops the inner one.
  const computation = Tracker.nonreactive(() =>
    Tracker.autorun(func)
  )
  onDestroy(() => {
    computation.stop()
  })
}
function trackServer (func) {
  return Tracker.nonreactive(func)
}

export const track = Meteor.isClient
  ? trackClient
  : trackServer

CaptainN commented 5 years ago

Even better - this one can react to reactive changes from svelte:

import { Tracker } from 'meteor/tracker'
import { onDestroy } from 'svelte'

function getTracker () {
    let computation
    return (func) => {
        // If we already have a computation, destroy it and start over
        if (computation) computation.stop()

        // Use Tracker.nonreactive in case we are inside a Tracker Computation.
        // This can happen if someone starts rendering inside a Computation.
        // In that case, we want to opt out of the normal behavior of nested
        // Computations, where if the outer one is invalidated or stopped,
        // it stops the inner one.
        computation = Tracker.nonreactive(() =>
            Tracker.autorun(func)
        )
    }
    onDestroy(() => {
        if (computation) computation.stop()
    })
}

Now we can recreate the computation if Svelte deps change:

<script>
    let count = 0
    let val = 0
    const track = getTracker();
    $: {
        track(function test () {
            count += val
        })
    }
    setTimeout(() => { count++ }, 1000)
    setTimeout(() => { val++ }, 2000)
</script>

<h1>{count} times!</h1>

I'll make a PR

Update: With deeper testing, this doesn't seem to work. It gets us reactivity to svelte deps, but as soon as the meteor stuff starts getting wired up, things get wacky. I'll keep at it, there's probably an elegant way to make this all play nice together.

red-meadow commented 4 years ago

@CaptainN, could you tell what went wrong with deep testing? I think, this is must have feature. Also I'm curious why you didn't use computation.invalidate() instead of computation.stop()?

CaptainN commented 4 years ago

Getting both reactive systems to work together is what's tricky. Getting each one to work alone is easy and simple. There were just a bunch of edge cases where the changes from either Svelte or Meteor wouldn't update the component. I still think there may be a way to get it to work, but I haven't had time to work on it recently.

computation.invalidate says that one of the internal dependencies has changed. computation.stop stops and ends the computation, and cleans up all the references to so it can be GCed, which is exactly what we want when the svelte component is unmounted. For external changes (svelte reactive changes) we need to stop and rebuild the computation, so that the new (changed svelte) values are enclosed, and the reactive data sources subscribed with the old values are unsubscribed.

red-meadow commented 4 years ago

But is there a need to rebuild the computation each time? To enable Svelte reactivity, the func function should take values from closure anyway. I changed your code a bit and it works with the same result:

function getTracker () {
    let computation
    onDestroy(() => {
        if (computation) computation.stop()
    })
    return (func) => {
        if (computation) {
            // If we already have a computation, invalidate it
            computation.invalidate()
        }
        else {
            // Use Tracker.nonreactive in case we are inside a Tracker Computation.
            // This can happen if someone starts rendering inside a Computation.
            // In that case, we want to opt out of the normal behavior of nested
            // Computations, where if the outer one is invalidated or stopped,
            // it stops the inner one.
            computation = Tracker.nonreactive(() =>
                Tracker.autorun(func)
            )
        }
    }
}

Probably I ignore more complex cases. The one I figured out is passing callback dynamically, like track(condition ? func1 : func2), but the logic can be just moved inside the callback.

CaptainN commented 4 years ago

Oh, I remember why. It was more about something in react - we needed the first-run to work synchronously with render when it really was the first run (and first render) and when the deps changed. What we were getting without doing that (by just invalidating) was that for 1 render pass, the old data, derived from the old deps values would persist. This caused subtle bugs in situations where a key was not provided, and the react reconciler reused existing component instances (such as inside react router's switch blocks).

I'm not sure the same need for synchronous is needed in Svelte in response to deps changes, but if it is, then it's better to recreate the computation, because it'll run in the same event loop. (I don't think invalidate causes the next tracker run to run in the same event loop - I'd have to look through the source.)

red-meadow commented 4 years ago

Yes, I think I understand. I tried this and got two consecutive page updates with invalidate() while only one with stop():

<script>
...
$: {
    b++
    track(() => { sum = a + b })
}
setTimeout(() => { a++ }, 2000)
</script>

<div>{b} {sum}</div>

red-meadow commented 4 years ago

Sorry for spamming the thread but isn't this a solution for synchronicity? Could it be related to any of the edge cases you mentioned?

return (func) => {
    if (computation) {
      // If we already have a computation, run callback synchronously
      computation._func()
    } else {
      computation = Tracker.nonreactive(() =>
        Tracker.autorun(func)
      )
    }
  }

CaptainN commented 4 years ago

I think we need a set of unit tests to validate any of these approaches. It's super easy to get everything working without worrying about reacting to Svelte changes. Writing tests for that is probably the place to start.

davidsavoie1 commented 4 years ago

I experimented with another approach to Svelte reactivity.

withTracker.js

import { Tracker } from "meteor/tracker";
import { onDestroy } from "svelte";

export default function withTracker(trackedFn) {
  const computation = Tracker.autorun(trackedFn);

  onDestroy(() => {
    computation.stop();
  });

  return computation;
}

App.svelte (script only)

import withTracker from "./withTracker.js";
import { Actions } from "/collections";

let today = false;
let completed = false;

let actions = [];
const actionsComputation = withTracker(() => {
  actions = Actions.find({ today, completed }).fetch();
});

$: {
  actionsComputation.invalidate([today, completed]);
}

withTracker returns the computation instance. The Svelte component then sets up a reactive statement that will invalidate it if any of the parameters change. computation.invalidate takes no argument, so I pass the parameters there for concision. That way, the dependencies are explicit.

What are your thoughts on this approach? It works very well so far...

zodern commented 4 years ago

I am experimenting with integrating with tracker by creating a svelte preprocessor.

Script tag in source file:

import { Apps } from '/imports/collections';
let count = 1;
let apps;

$m: apps = Apps.find().fetch();
$: doubled = count * 2;

After preprocessor runs:

import { getUniqueId as _m_getUniqueId, withTracker } from "meteor/svelte:compiler";
const _mId0 = _m_getUniqueId();
import { Apps } from '/imports/collections';

let apps;

$:
withTracker(_mId0, () => {
  apps = Apps.find().fetch();
});
$: doubled = count * 2;

The methods imported from svelte:compiler are:

const { onDestroy } = require('svelte');

const computations = new Map();

function withTracker (id, func) {
  if (typeof id === 'function') {
    let computation = Tracker.autorun(id);

    onDestroy(() => {
      computation.stop();
    });

    return computation;
  }

  let computation = computations.get(id);
  if (!computation) {
    computation = Tracker.autorun(func);
    computations.set(id, computation);

    onDestroy(() => {
      computation.stop();
      computations.delete(id);
    });
  } else {
    computation.invalidate();
  }

  return computation;
}

let currentId = 0;
function getUniqueId() {
  return ++currentId;
}

module.exports = {
  withTracker,
  getUniqueId
}

Labeled statements named $m are rewritten to use Tracker.autorun and have a $ label. Svelte is then able to compile them as a normal reactive statement, detecting non-tracker dependencies and handling invalidation for us. I had considered other options for the label name, but $m is short and easily distinguishable from $.

The one challenge is when svelte injects variable declarations for reactive statements. For example, in the file above, it would add let doubled to the compiled output since the reactive statement is an assignment, and the variable wasn't declared in the component. To support this feature, it will have to be implemented in the preprocessor for $m statements.

I am not sure of the next steps. Would it make sense to submit this as a PR to meteor-svelte? Or would it be better to keep it as a separate compiler package that supports the additional syntax?

rdb commented 4 years ago

Hi @zodern, where is your code uploaded currently?

zodern commented 4 years ago

I haven't made it public yet. There are some things that I want to improve or test first.

rdb commented 4 years ago

This appears to be a very minimalistic way to do two-way reactivity, wrapping the Meteor computation as a Svelte store, implementing the store contract to let Svelte stop the subscription automatically as needed.

This is the only bit you need:

tracked = fn => ({subscribe(set) {
    const computation = Tracker.autorun(() => set(fn()));
    return computation.stop.bind(computation);
}});

You use it like so:

<script>
let selectedUserId;

$: selectedUser = tracked(() => Meteor.users.findOne(selectedUserId));
</script>

<h1>Selected {$selectedUser.username}</h1>

This will rerun when selectedUserId is changed by Svelte, but also update automatically whenever the database entry is changed by Meteor.

It would probably also be useful if Meteor cursors returned by find() could be used as stores directly, by monkey patching Cursor with a subscribe function that will use observeChanges to update the stored array.

rdb commented 4 years ago

Making cursors conform to the store contract to make them reactive out of the box via the $ operator is pretty easy. Here is a gobsmackingly simple todos app which adds/deletes to Mongodb, and the results show up reactively:

<script>
$: todos = Todos.find();

let todoName;
</script>

{#each $todos as todo}
  <div>To do: {todo.name} <button on:click={() => Todos.remove(todo._id)}>X</button></div>
{/each}

<form on:submit|preventDefault={() => Todos.insert({name: todoName})}>
  <input type="text" bind:value={todoName} />
</form>

This is the (minimally tested) monkey patch to Cursor that you need to make this happen:

Mongo.Cursor.prototype.subscribe = function(set) {
    let result = [];
    set(result);
    const handle = this.observe({
        addedAt: (doc, i) => {
            result = [...result.slice(0, i), doc, ...result.slice(i)];
            set(result);
        },
        changedAt: (doc, old, i) => {
            result = [...result.slice(0, i), doc, ...result.slice(i + 1)];
            set(result);
        },
        removedAt: (old, i) => {
            result = [...result.slice(0, i), ...result.slice(i + 1)];
            set(result);
        },
        movedTo: (doc, from, to) => {
            result = [...result.slice(0, from), ...result.slice(from + 1)];
            result.splice(to, 0, doc);
            set(result);
        },
    });
    return handle.stop.bind(this);
};

Note however, if you plan to pass data to a TodoItem component, you need to mark TodoItem as immutable or use destructuring, or Svelte will rerender all of the items when a single one is added. (This is mostly a limitation of Svelte rather than of this approach, but it was pitfall for me, so I thought I'd mention it.)

rdb commented 4 years ago

I really think that svelte-tracker should at the very least have the ability to auto-stop any subscriptions or Tracker.autorun computations that were made in the component. The following code patches Meteor.subscribe and Tracker.autorun, obviating the need for any added calls or explicit onDestroy hooks (though, note it does not add inter-reactivity, which is a separate matter): https://gist.github.com/rdb/e0d04b8bee673371b4e3741ac1426c03

As an added bonus, the code above patches Meteor.subscribe to allow you to use subscriptions with await blocks, so the result is that you can do stuff like this, and it "just works":

{#await Meteor.subscribe('allTodoItems')}
  <p>Loading todo items…</p>
{:then}
  <p>Todo items:</p>
  …  
{/await}

@klaussner I think that this, and possibly the change in my previous post (which patches Mongo.Cursor to act as a Svelte store), are fairly basic features that I think a Meteor Svelte integration absolutely needs to have. Would you be open to reviving the svelte-tracker package for this, or should this go into the main package? I would be happy to do the work for this.

rdb commented 4 years ago

For now, I have published a rdb:svelte-meteor-data package that contains some of the suggested conveniences and integrations plus a useTracker function that behaves as a Svelte store, as well as a useSession for Session variables: https://atmospherejs.com/rdb/svelte-meteor-data

I will continue to experiment with this and add to it (suggestions are quite welcome), but I would be happy to see this eventually merged into this package or into another package in the svelte namespace.

CaptainN commented 4 years ago

@rdb I wonder whether we could patch the Meteor Cursor in a way that we could extend for Svelte and React. These types of closer integrations are exactly what I've been looking at on the React side too.

rdb commented 4 years ago

I have not used React in a while, but if there is a React package that adds automatic management of RxJS-style observables (ie. implementing a subsribe/set interface) then my patches will likely work with that.

Though since Svelte is a compiler, it can get away with doing "svelte" things like the $ short-hand, and it's probably not possible to get as close to that with React.

CaptainN commented 4 years ago

I just meant making sure changes to the Cursor interface could be generic enough to be useful by multiple platforms. I don't like Monkey patching because you never know what else users are doing. It wouldn't be great, but I can imagine a user implementing multiple front ends - I've seen it with React/Blaze - it's likely some will try React/Svelte. The main thing is to make sure things don't break is where I'm headed.

Actually, I wouldn't call that a Monkey patch at all, which I think implies changing something rather than adding to - this is an addition. Pretty neat - I think that could be useful for what I'm thinking about for the react stuff.

rdb commented 4 years ago

Oh sure. I would definitely be interested in splitting off eg. the Cursor changes into a separate, non-Svelte-specific package that can be used by other approaches as well.

I'm trying to avoid too much monkey-patching, but if other approaches fail, I don't mind it as much as long as it's somewhat maintainable and as long as it does not break other libraries that want to monkey-patch the same function. We can later consider suggesting changes upstream to make it cleaner.

In the meantime, I'm continuing to work on finding a way to get both component initialization and update code wrapped inside a Tracker.Computation. Doing this would make a bunch of things "just work". Methods like observeChanges would automatically stop on component destruction (without needing a custom patch for every such method), and we could make them invalidate properly if called from component.$$.update, which means that Tracker.autorun could work properly inside $:.

I'm struggling to find a good way to hook in to wrap these cases without a monstrously ugly hack, like overwriting internal Svelte functions or replacing Meteor's Tracker.currentComputation with a custom property, all of which would be fragile and undesirable.

I'll continue to poke around in the Svelte code to make sure I'm not missing an elegant approach, but I think it comes down to the choice between:

Ship a custom patched version of the Svelte runtime. This is what the Vue integration ended up doing, and offers us the most flexibility, but I was hoping to avoid this.
Patching svelte:compiler to insert or redirect some calls in the generated code if it detects that rdb:svelte-meteor-data is added. Not sure how to do this cleanly without, again, modifying upstream Svelte.
Convince Meteor to accept a patch to Tracker that will allow us to hook into it cleanly (doubtful)
Convince Svelte to give us a clean way to wrap calls to component code (wouldn't count on it)

It's not the end of the world if we can't do this, but it would certainly be a boon if we could.

rdb commented 4 years ago

I got Tracker.autorun to work inside $: (package updated). However, there are some caveats. If the autorun is invalidated right after the Svelte state is updated, it cannot know which Svelte state change requires which autorun to be rerun (without added instrumentation), so it instead simply invalidates all the Svelte $: computations so that all the autoruns are hit again. This will result in unnecessary (though probably harmless) extra updates.

I have half a mind to simply throw an error if Tracker.autorun is run inside $: and tell people to use useTracker instead, which works well in any context, but there's something to be said for having all basic Meteor constructs "just work" in Svelte, even if it is occasionally not as efficient.

zodern commented 4 years ago

I created a PR with the preprocessor: https://github.com/meteor-svelte/meteor-svelte/pull/38

klaussner commented 4 years ago

Thank you all for taking the time to experiment with these interesting ideas! I haven't thought about any of these things when I opened this issue and I think it's clear that some type of Tracker integration is needed. After going through the comments, I prefer the Svelte store solution by @rdb because it looks like it's idiomatic for Svelte and the most easy one to implement.

However, I'm a bit skeptical about patching the Svelte store contract into Mongo.Cursor because after an assignment like $: todos = Todos.find();, $todos is not a cursor. This could be confusing because people might try to call functions like fetch on $todos.

Maybe we can make this cursor to store conversion more explicit, for example by implementing it in tracked and documenting that tracked always returns a Svelte store.

<script>
  let user = ...

  $: todos = tracked(() => {
    Meteor.subscribe("todos", user);
    return Todos.find(); // `tracked` converts this cursor to a store.
  });
</script>

{#each $todos as todo (todo._id)}
  <Todo todo={todo} />
{:else}
  Loading todos...
{/each}

This also has the advantage that related calls such as Meteor.subscribe and find can be grouped together. Another alternative would be a cursorToStore function that could be used like this: $: todos = cursorToStore(Todos.find());

What do you think? 🙂

rdb commented 4 years ago

Well, todos will still be a cursor that you can call fetch() on. It's $todos that amounts to (effectively) a reactive fetch. I struggle to imagine a line of thought that could lead people to consciously try and use $ and .fetch() at the same time.

Though, I could see an argument being made that having cursors act like stores could be a little magical. People might similarly expect collection.findOne() to work reactively, by extension. Your suggestion of allowing a cursor return value from tracked() does make sense, the only problem being that every time the reactive dependency changes it would effectively amount to a re-fetch(). Maybe this is not a big problem. I'll experiment with it.

Note that most of the previous suggestions and more are already implemented at rdb:svelte-meteor-data so feel free to take code from that as needed. I'd be happy to do the work of moving any desired features to this repo as needed.

Regarding naming, I had used useTracker after the similar React hooks, but upon further reflection, it might be better to use either tracked or trackable, which looks cleaner and more similar to the convention used for Svelte stores.

rdb commented 4 years ago

@klaussner: I began to implement it but quickly realised that having tracked() behave as you suggested is not really implementable in an elegant way. The store would either have to return as value:

The cursor itself (which would be non-reactive, and non-iterable, negating the whole purpose), or
The result of cursor.fetch(), which would mean that the code would no longer be returning a cursor, and other operations like .count() would no longer work as expected, or

Another store, which means you would need to unwrap it like this, which is very unwieldy:

$: todosCursorStore = tracked(() => Todos.find());
$: todosCursor = $todosCursorStore;
$: todos = $todosCursor;

(You might alternatively suggest that tracked() could actually return a subscribable cursor directly, but besides being less efficient—since any database change would require invalidating the autorun—it would break if your tracked() returned a different cursor depending on some reactive value.)

If one wanted to go down this route, one would be better off implementing some sort of a fetched (or fetchable) store that would effectively (though possibly more efficiently) work like this:

// this:
const taskStore = fetched(Tasks.find(...));
// would work like this does already:
const taskStore = tracked(() => Tasks.find(...).fetch());

Though, this is functionally the same as the option of having cursors be implicitly subscribable, only with some extra boilerplate. We could go for that if the extra boilerplate is considered less "magical", I suppose, but the fact that cursors are also implicitly iterable in Blaze makes me tend towards my implicit option after all.

meteor-svelte / svelte-tracker

Deprecate this package for Svelte v3? #3

withTracker.js

App.svelte (script only)