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

RFC: <svelte:inject> #1567

Closed
Rich-Harris opened this issue Jun 28, 2018 · 14 comments
Closed

RFC: <svelte:inject> #1567

Rich-Harris opened this issue Jun 28, 2018 · 14 comments

Comments

@Rich-Harris
Copy link
Member

The motivation behind this idea is to facilitate nested routes in Sapper, and this RFC is written with that particular use case in mind. If it seems overly Sapper-specific and needs to change, then I'm interested in thoughts as to how.

The idea is that you could have a page like this...

<!-- routes/settings/index.html -->
<div class="settings">
  <SideBar/>
  <div class="submenu">
    <svelte:inject/>
  </div>
</div>

and another like this...

<!-- routes/settings/notifications.html -->
<h2>Notifications settings</h2>
<label>
  <input type=checkbox bind:checked>
  show me notifications
</label>
<button on:click="save()">save</button>

...and the second component would be injected into the first at the site of the <svelte:inject>, were you to visit /settings/notifications.

If there was a routes/settings/password.html file, it would be injected upon visiting /settings/password and so on.

In effect, it would be similar to this...

<svelte:component this={Submenu}/>

...except that Submenu isn't part of the the parent component's state; it would come from some global registry. (There are a number of tangible benefits to this beyond ergonomics — it enables more granular code-splitting, for example.)

Implementation

If we accept two constraints...

  1. Components are compiled with shared: true
  2. A component can only have one <svelte:inject>

...then a plausible solution would be to have the global registry be imported from inside svelte, and for the component's filename to be the registry key:

// adapted from https://svelte.technology/repl?version=2.8.1&gist=9f054b9e5711d36f35f5c9155c6f3fc8
+import * as injector from 'svelte/injector.js';

const file = "routes/settings.html";

function create_main_fragment(component, ctx) {
  var switch_instance_anchor;

+  var switch_value = injector.get(file));

  function switch_props(ctx) {
    return {
      root: component.root,
      store: component.store
    };
  }

  if (switch_value) {
    var switch_instance = new switch_value(switch_props(ctx));
  }

  return {
    c: function create() {
      switch_instance_anchor = createComment();
      if (switch_instance) switch_instance._fragment.c();
    },

    m: function mount(target, anchor) {
      insertNode(switch_instance_anchor, target, anchor);

      if (switch_instance) {
        switch_instance._mount(target, anchor);
      }
    },

    p: function update(changed, ctx) {
+      if (switch_value !== (switch_value = injector.get(file))) {
        if (switch_instance) switch_instance.destroy();

        if (switch_value) {
          switch_instance = new switch_value(switch_props(ctx));
          switch_instance._fragment.c();
          switch_instance._mount(switch_instance_anchor.parentNode, switch_instance_anchor);
        }
      }
    },

    d: function destroy(detach) {
      if (detach) {
        detachNode(switch_instance_anchor);
      }

      if (switch_instance) switch_instance.destroy(detach);
    }
  };
}

This would cause <svelte:inject> to update correctly when the parent component was created, or when any state changed inside the parent component.

It would not work if the value of injector.get(file) changed independent of a state change. For that, we would need to add some code to the component constructor:

+import * as injector from 'svelte/injector.js';

function App(options) {
  init(this, options);
  this._state = assign({}, options.data);

  if (!options.root) {
    this._oncreate = [];
    this._beforecreate = [];
    this._aftercreate = [];
  }

  this._fragment = create_main_fragment(this, this._state);

  if (options.target) {
    this._fragment.c();
    this._mount(options.target, options.anchor);

    this._lock = true;
    callAll(this._beforecreate);
    callAll(this._oncreate);
    callAll(this._aftercreate);
    this._lock = false;
  }

+  this.on('destroy', injector.register(file, () => {
+    this._fragment.p({}, this._state);
+  }));
}

The registry would look something like this:

// svelte/registry.js
const callbacks = {};
let components = {};
let promise;

export function set(_) {
  components = _;

  // need to allow state-change-driven updates
  // to happen first, lest things get out of sync
  Promise.resolve().then(() => {
    Object.keys(_).forEach(file => {
      if (callbacks[file]) callbacks[file]();
    });
  });
}

export function get(file) {
  return registry[file];
}

export function register(file, callback) {
  callbacks[file] = callback;

  return () => {
    callbacks[file] = null;
  };
}

Inside Sapper, the components would be updated on navigation:

// sapper/runtime.js
import * as injector from 'svelte/injector.js';

async function navigate(route) {
  const { Page, children } = await route.load();
  injector.set(children);
  
  // ...
}

Does this seem workable? And is <svelte:inject> the right solution?

(Regardless of what we decide here, it might make sense to figure out how to handle transitions between values of xyz in <svelte:component this={xyz}> before going too far with the implementation, as it will no doubt be relevant.)

@TehShrike
Copy link
Member

<svelte:inject> sounds great, but I wouldn't want the injector to be internal to Svelte - how about something like this:

const parent = new Parent({
	target: document.getElementById('target')
})

const child = parent.inject(Child, {
	data: {
		cool: true
	}
})

and maybe support component instances as targets for sugar:

const child = new Child({
	target: parent,
	data: {
		cool: true
	}
})

Sapper can maintain its own injection scheme, ASR can do its thing, and any other router should be able to work, as long as Svelte handles the "put this thing inside you. Okay, now remove it" bits.

I dunno what the SSR API should be.

"A component can only have one svelte:inject" sounds totally reasonable to me. With the API I imagine, requiring shared: true doesn't seem necessary though.

@Rich-Harris
Copy link
Member Author

I should have fleshed this out more in the original comment, but there's a couple of reasons I didn't initially go down that road. First, you might have props and children:

<svelte:inject foo={bar}>
  <MrClippy message="It looks like you're trying to {goal}"/>
</svelte:inject>

It feels like that stuff is easier to coordinate if it's declarative.

Secondly, inside Sapper we only actually have a reference to a single component instance — the top-level app component. We don't have a way to get a reference to parent.

I do like where your head's at though. Will have to think about whether those issues are solvable.

@TehShrike
Copy link
Member

Oh cool, yeah, the ability to pass things to injected components is great, I thought you were specifically avoiding that, like with slots (can we get that behavior with slots? 😃).

But shouldn't that still work? Maybe I underestimate the difficulty of merging the construction options passed to inject with whatever construction options components pass to their children.


I know Sapper only deals with the top-level instance now, but ASR holds on to instances at every level for a couple reasons.

Routers that want to, say, change the injected component at the lowest level of the hierarchy would presumably need to hold on to every instance.

// page load at /app/settings/profile
const app = new App()
const settings = app.inject(Settings)
const profile = settings.inject(Profile)

// route changes to /app/settings/emails
settings.inject(Emails) // (parent components aren't touched)

@tivac
Copy link
Contributor

tivac commented Jun 28, 2018

Agreed on the desire to programmatically inject new children, it seems like that'll be way easier to integrate w/ other libraries.

@Rich-Harris
Copy link
Member Author

can we get that behavior with slots?

This is more like

<svelte:component this={Subroute} foo={bar}>
  <MrClippy message="It looks like you're trying to {goal}"/>
</svelte:component>

— IOW not really a new thing from that point of view. (I remain convinced that <slot thing={lol}> is a Bad Idea.)

@ekhaled
Copy link
Contributor

ekhaled commented Jun 29, 2018

The idea of resolving components from a shared global registry is a great one.
However, this seems a bit too specific to sapper.

What are the reasons we can't just rework <svelte:component /> to also be able to do this?
From what I understood, it would be re-using a lot of the logic for <svelte:component /> anyway.

@Rich-Harris
Copy link
Member Author

I tried to figure out a way that it could just use <svelte:component>, but where would it get its definition (i.e. its this) from?

There are three options:

  1. It comes from the component state (<svelte:component this={Child}>), which would mean that we'd need to somehow get a reference to the parent component (same project as with parent.inject(...), for which I'm interested in any ideas)
  2. It comes from a store (<svelte:component this={$childComponents['settings']}>), which could get in the way of the app's existing store and is fundamentally somewhat brittle and boilerplatey
  3. It comes from a separate global registry, which would involve inventing a new thing — which may as well be <svelte:inject>

Another nice thing about <svelte:inject> is that it cleanly declares your intention — it means that you can differentiate between 'this is a page that should inject its child route upon navigation' and 'this is a page that should be replaced with its child route'. The presence of <svelte:inject> at compile-time also allows us to create smarter manifests that, in turn, make lazy-loading very straightforward — I think this would be a lot harder without a dedicated primitive:

// app/manifest/client.js
const imports = [
  () => import('../../routes/index.html'),
  () => import('../../routes/settings/index.html'),
  () => import('../../routes/settings/notifications/index.html'),
  ...
];

const routes = [
  {
    pattern: /^\/?$/,
    params: () => ({}),
    load: () => {
      return imports[0]().then(Page => {
        return { Page, children: {} };
      });
    }
  },
  {
    pattern: /^\/settings\/?$/,
    params: () => ({}),
    load: () => {
      return imports[1]().then(Page => {
        return { Page, children: {} };
      });
    }
  },
  {
    pattern: /^\/settings\/notifications\/?$/,
    params: () => ({}),
    load: () => {
      return Promise.all([imports[1](), imports[2]()]).then(([Page, _1]) => {
        return {
          Page,
          children: {
            'settings/index.html': _1
          }
        };
      });
    }
  },
  // ...
];

I do like the idea of having an API for this stuff, as an alternative to a magic registry (though, to be clear, the registry isn't Sapper-specific — any app could do injector.set({ 'some/file.html': Thing })). Getting a reference to the parent component is a challenge though.

One more wrinkle. What would happen if the <svelte:inject> weren't in the page itself?

<!-- routes/settings/index.html -->
<div class="settings">
  <SideBar/>
  <Submenu/>
</div>
<!-- routes/settings/_components/Submenu -->
<div class="submenu">
  <svelte:inject/>
</div>

That would make it a little harder to use filenames as registry keys.

@ekhaled
Copy link
Contributor

ekhaled commented Jun 29, 2018

I understand where you are coming from (between and wall and a hard place 😄).

I have a question though.
A page can have multiple components, each component can have a <svelte:inject /> in it.
How are you planning to figure out where to inject the component?

@TehShrike
Copy link
Member

What would happen if the <svelte:inject> weren't in the page itself?

I would lean towards:

  1. assume it's a leaf component, and everything is cool
  2. if there are directories with child components, that assumption seems incorrect, so it should probably throw a build-time error to warn the developer

@TehShrike
Copy link
Member

hmm, my last post was too Sapper-centric. Svelte doesn't know about routes/directories.

Put a Svelte-centric way, I would want parent.inject(Child) (or whatever the "render components with these children" function ends up being) to throw an error if there was no <svelte:inject> in the template.

@Rich-Harris
Copy link
Member Author

A page can have multiple components, each component can have a <svelte:inject /> in it.
How are you planning to figure out where to inject the component?

Two possibilities:

  1. We only allow pages to have <svelte:inject> — Sapper throws an error if foo/_components/Thing.html contains one
  2. We somehow determine that foo/_components/Thing.html is a dependency of foo/index.html (and only that page) and Sapper throws an error if a) other things depend on it or b) foo/_components/OtherThing.html also has a <svelte:inject>. Gets a lil complex though

@silentworks
Copy link

On the naming of this special tag, I would opt for <svelte:children>, I think its clear what is intended to show up inside of this tag.

@Rich-Harris
Copy link
Member Author

I'm starting to feel a little uneasy about this proposal. Apart from the magic involved (I'm not strictly opposed to magic if it significantly improves the developer experience, but I do think there should be a presumption against magic) there are some issues with it.

Firstly, it would realistically only work if <svelte:inject> was restricted to page components (option 1 in my previous comment). Option 2 would be pretty confusing, I think. That feels like a bit of a straitjacket.

Secondly, I'm not sure it does what we want. Consider a very basic example — an app with three routes:

  • /
  • /blog
  • /blog/[slug]

Neither routes/index.html nor routes/blog/index.html could contain <svelte:inject>, because there's no way to say 'if there is no child route, render this content' (i.e. the home page blurb on /, or a list of posts on /blog). So it's not like we could start putting layout in routes/index.html and get rid of App.html, which was part of the thinking behind this idea.

So I think we might need to go back to the drawing board. My previous idea (sveltejs/sapper#262 (comment)) is starting to appeal a bit more — I just wonder if there's a smart way to retain some of the benefits we talked about (such as more granular code-splitting, non-destruction of unchanged non-leaf components) and get rid of some boilerplate.


Separately, it occurs to me that having an API for this would be a bit unnecessary, since it basically already exists:

<svelte:component this={SomeWellKnownProperty}/>
parent.set({ SomeWellKnownProperty: Child });

@Rich-Harris
Copy link
Member Author

Here's a better idea: sveltejs/sapper#262 (comment)

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

5 participants