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

hotReload: true prevents page navigation #43

Closed
Rich-Harris opened this issue Mar 4, 2018 · 12 comments · Fixed by ekhaled/svelte-dev-helper#2
Closed

hotReload: true prevents page navigation #43

Rich-Harris opened this issue Mar 4, 2018 · 12 comments · Fixed by ekhaled/svelte-dev-helper#2

Comments

@Rich-Harris
Copy link
Member

I haven't looked into this at all yet, but it appears that hotReload: true is breaking page navigation in Sapper. My hunch is that it doesn't play nicely with components that have already been destroyed with component.destroy(). cc @ekhaled

@ekhaled
Copy link
Contributor

ekhaled commented Mar 4, 2018

destroy is proxied through.. I'm not sure but I'll have a look.
Where's the page navigation code in sapper?

@ekhaled
Copy link
Contributor

ekhaled commented Mar 4, 2018

Think it might be to do with hydrate support

@Rich-Harris
Copy link
Member Author

That sounds plausible... the navigation code is in https://github.com/sveltejs/sapper/blob/master/src/runtime/index.ts, specifically this function

@ekhaled
Copy link
Contributor

ekhaled commented Mar 4, 2018

cheers, I'm on it right now, will keep you posted

@Rich-Harris
Copy link
Member Author

awesome, thank you

@ekhaled
Copy link
Contributor

ekhaled commented Mar 4, 2018

I might need some help with svelte/sapper internals.

For example (this is from sapper-template), there is a component routes\index.html.. it contains <Layout>. While _mount() is called in <Layout>, why is _mount never called on routes/index.html ?

@Rich-Harris
Copy link
Member Author

_mount is a semi-private API that a parent uses to control exactly when/where its child components are mounted. routes/index.html is a top-level component, so it mounts itself, but <Layout> is a child of routes/index.html, which means routes/index.html is responsible for mounting it using layout._mount.

If you go here https://svelte.technology/repl?version=1.55.0&example=nested-components and click the input/output toggle, you'll notice nested._mount on line 24, but on line 58 the App mounts its own fragment without calling _mount (though it could be rewritten so that it always did go through _mount, now that I think about it)

@ekhaled
Copy link
Contributor

ekhaled commented Mar 4, 2018

Thanks, makes a lot of sense...
I hijack the _mount to do some book keeping.

Is this snippet the self-mounting code?:

if (options.target) {
		if (options.hydrate) throw new Error("options.hydrate only works if the component was compiled with the `hydratable: true` option");
		this._fragment.c();
		this._fragment.m(options.target, options.anchor || null);

		........
}

@ekhaled
Copy link
Contributor

ekhaled commented Mar 4, 2018

yeah, sorry.. ignore me.. you pointed out the line number

@ekhaled
Copy link
Contributor

ekhaled commented Mar 4, 2018

just to keep you updated... I have skirted around the self-mounting components issue.
However, it would be better to make them go through _mount as you suggested.

Still trying to figure out why preload() breaks rendering. Something to do with data not getting passed through properly.

@ekhaled
Copy link
Contributor

ekhaled commented Mar 4, 2018

Hi,

Two problems:

  1. Self mounting components throwing errors on destroy (Because they assume they were mounted with _mount).
  2. preload() static method not being forwarded by the proxy.

I have fixed both in v1.1.4. And it seems to work perfectly with Sapper.

However, there are still some edge cases where hot-reload does not repaint top-level components after a change.

If you can make it so that even top-level components also go through _mount, then these edge cases will be resolved too (hopefully).

@Rich-Harris
Copy link
Member Author

Everything seems to be working well with this, the latest Sapper (and sveltejs/sapper-template#40), so I'll close this. Thank you so much!

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 a pull request may close this issue.

2 participants