woutdp / live_svelte

Svelte inside Phoenix LiveView with seamless end-to-end reactivity
https://hexdocs.pm/live_svelte
MIT License
1.01k stars 38 forks source link

Pass live api through context not a prop #70

Open vonagam opened 10 months ago

vonagam commented 10 months ago

Right now live api is exposed as a prop. And we pass it to all rendered components, even those who do not ask for it, which causes svelte to complain about it.

A viable alternative is to pass it not as a prop, but through context. Which will remove the warnings and ease access for nested components to live api. It is a breaking change through.

// before
export let live

// after
import {getLive} from "live-svelte"
const live = getLive()

getLive is typed which is also a bonus.

To make it work, two and half lines with setupLive call have to be added to app.js (so no action from new users, but migrating users need to add them):

import {getContext} from "svelte"
import {getHooks, setupLive} from "live_svelte"
setupLive(getContext)

Edit: updated the description to reflect current state.

woutdp commented 10 months ago

First impression is that I like this, haven't looked at the code but the idea is sound.

Nested components might need access to the live api, currently you need to pass it manually through every prop, or use a context yourself. This solves that.

Also, conceptually it's a more sound idea than using props. Props should be data, interface code should be accessed as a library, not passed around as a prop.

I also prefer the latter solution, using a context still feels like it's data instead of code.

Let me think about it a bit again before committing to this :)

woutdp commented 10 months ago

As a counterargument, export let live is dead simple. Context makes it slightly more complex, it's a small thing but I do like the UX at the moment of simply doing export let live.

Using it as a library (util) is the best solution though. Conceptually best, and easy to understand.

vonagam commented 10 months ago

Let me think about it a bit again before committing to this :)

Of course, no pressure, I just saw Component api page for unrelated reason and when it came to context option I was like "Huh, why not use it instead?".

CommonJS support is a blocker for the util right now (as I see it), but I feel like nodejs package will need to be replaced at some point, it does not seem to be mantained. I also remember you saying on elixir forums that performance of ssr in development wasn't good, it might be because it currently invalidates require cache on every call, with ESM one would use something like hot loader (I used this one for example) and I'm interested if there will be any difference in speed.

woutdp commented 10 months ago

but I feel like nodejs package will need to be replaced at some point, it does not seem to be mantained. I also remember you saying on elixir forums that performance of ssr in development wasn't good

Definitely. I'm working on a project (https://territoriez.io/) that uses LiveSvelte quite deeply and I've struggled a bit with SSR. For example it seems there's some memory leakage going on. So I've disabled it for now.

https://github.com/woutdp/live_svelte/issues/36 should be an all encompassing issue that replaces it with something better.

So I'm invested in fixing it at some point. But also waiting it out just a little until it's the right time to tackle it. Working with it in production is giving me a good feel of what the pain points still are for LiveSvelte.

vonagam commented 10 months ago

Added half-way implementation for getLive. It requires getContext to be passed as an argument to it, so no dependencies. It is to improve typed experience a little and is also future-migration-friendly when it will not need the argument. Util's usage is optional.

// typed basic
import {getContext} from 'svelte'
import {Live} from 'live-svelte'

const live: Live = getContext("live-svelte")

// typed util
import {getContext} from 'svelte'
import {getLive} from 'svelte-live'

const live = getLive(getContext)
dev-guy commented 10 months ago

I like this. Eliminating the Svelte warnings is a big win IMO since the fewer warnings my team has to ignore, the less likely they are to miss the important ones. SSR, among its other issues (speed, scalability, another server-side library), is unstable for me and I submitted an issue about it. I don't understand how it could be working for other LiveSvelte users. I don't see myself using SSR with LiveSvelte because if I didn't require heavy-duty client-side code, I would use vanilla Phoenix.

woutdp commented 10 months ago

SSR definitely needs a rework. It can be slow and seems to have some memory issues.

I'm not 100% convinced of the proposed solution though.

If we can do something like this:

import {getLive} from "live-svelte"
const live = getLive()

I'd be happy to integrate that.

vonagam commented 10 months ago

For that a change of ssr node provider needs to happen first. To one that supports esm since new svelte does not support cjs.

Also would recommend to add js build artifacts to gitignore. When this package will start to import svelte it will increase the size of the js output. I don't think there is much gain from committing those builds, but they do pollute the history and increase size of the repo. Or am I missing something about why they need to be shown?

woutdp commented 10 months ago

Also would recommend to add js build artifacts to gitignore.

That's right, gitignored and removed them.

vonagam commented 10 months ago

Hm. How about this: New optional options argument for getHooks with type of {prop?: boolean, context?: boolean}. With default behavior matching {prop: true, context: true}. For a person to disable warnings they would need to pass {prop: false} and use only context access. I don't know if there is any downside to providing a context but put an option to disable it for consistency, can remove it and have only simple {prop?: boolean}.

vonagam commented 10 months ago

Nah, scrap this. Back to context only. Made this work:

import {getLive} from "live-svelte"
const live = getLive()

For that added setupLive method that needs to be called in app.js with getContext from svelte. So those lines were added to the template:

import {getContext} from "svelte"
import {getHooks, setupLive} from "live_svelte"
setupLive(getContext)

Could have done it from the start but wanted to avoid usage of global state. But, since svelte itself uses it and there can only be one svelte, should be ok.

vonagam commented 9 months ago

I've updated the description with current state of the PR. Do you have any reservations about current implementation?

woutdp commented 9 months ago

Saw this but not a 100% sure, if we can get rid of manually passing around the context in the setup then we're good to go.

dev-guy commented 9 months ago

vonagam Why doesn't live_svelte do import {getContext} from "svelte" ? Looks like you're avoiding adding a direct dependency between live_svelte and svelte. I'm curious why. Does that throw a runtime error in the browser despite the fact that getContext isn't called until later? You solution here might also be the solution to #71

vonagam commented 9 months ago

I'm curious why.

Svelte 4 does not support cjs and current runtime expects cjs.

if we can get rid of manually passing around the context

I don't think we can get better than this at the moment. It is just 2 lines in the template.

woutdp commented 9 months ago

I think we should replace Node with Bun before we do this. Bun has reached 1.0 and should be compatible. This will avoid the cjs stuff. This should probably be a separate package inside the Elixir ecosystem. Bun is not available as a library, only as an executable. It has support for ffi and it might be possible to call Elixir code from javascript... There's a huge opportunity here if someone feels like picking up this... my spidey sense it tingling, some really cool things could be built from this I feel. Worth to explore.

alexkornitzer commented 7 months ago

Right so I have had a look at this today because Svelte 5 beta has dropped and I think its going to pair even better with LiveView. It could even solve the slot issue as they have been deprecated in favour of fragments if I have understood correctly.

Bun can easily replace esbuild due to this work https://github.com/crbelaus/bun. But getting Bun to work has been a challenge as its build command is not fully compatible with esbuild as such esbuild-plugin-import-glob and esbuild-svelte won't work without patches as typically they make use of features that are not implemented. Furthermore a concept like the nodejs library is required to execute Bun for SSR, as a hack I just used System.cmd.

Getting the Svelte 5 to render a basic component was pretty easy, just had to handle the breaking changes with components not being classes anymore: https://svelte-5-preview.vercel.app/docs/breaking-changes#components-are-no-longer-classes

Torkan commented 4 months ago

Sorry to resurrect this old discussion, but some of the earlier posts were sort of relevant to what I've been experimenting on 😅

With some minor tweaking, I've managed to get both SSR and client-side working with ES-modules (still using nodejs), using esbuild to compile each svelte component/page entry point.

I've also decoupled the es-modules from the LV-hooks (currently live_svelte needs to import all of them when setting up the hooks), instead importing them dynamically by using a data-attribute with the location of the respective js-module.

The same approach should also work fine when Svelte 5 lands.

I can create a repo demonstrating all of this if you're interested @woutdp, and if you like what you see I might be able to set off enough time to create a PR sometime in the not too distant future as well 😅

woutdp commented 4 months ago

@Torkan Awesome work, repo showing this off is useful! Would definitely be up for merging a PR that implements this too :)

Torkan commented 4 months ago

@woutdp: https://github.com/Torkan/live_svelte_ssr