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

Some breaking changes for 0.11 #65

Closed vonagam closed 10 months ago

vonagam commented 11 months ago

Nothing too drastic but still will require small simple migrations steps from users.

Move ssr build from priv/static/assets into priv/svelte

priv/static/assets files are exposed through Plug.Static, priv/static files are digested and compressed with phx.digest task. Neither of those things need to apply to ssr build, so changed it location to priv/svelte.

Migration: a user will need to update optsServer.outdir in build.js and add priv/svelte to gitignore.

Set dev in svelte compiler options

Activate dev mode in non-deploy builds for svelte. It enables various things like @debug tag and logs warnings on improper usage of components. (Not a breaking one since it applies only to assets/copy.)

Instead of pushEvent/pushEventTo pass live hook with live prop

Unless user exports both pushEvent and pushEventTo svelte complains about passing of unknown props in dev mode. It is annoying and unfortunately there is no way to check if a component exports those... To limit that annoyance replace pushEvent/pushEventTo with a single prop live and pass live hook instance into it so now to remove that warning exporting that prop is enough. It also gives a user access to more of live view capabilities - handleEvent/removeHandleEvent/upload/uploadTo.

Migration: need to replace export let pushEvent with export let live and pushEvent calls with live.pushEvent.

Replace render/exportSvelteComponents with getRender

exportSvelteComponents was needed because there was usage of require and delete require.cache in ssr render function. But that usage has been removed already, so api can be changed to more clean one. (That commit also contains somewhat subjective clean up of internal js.)

Migration: need to replace three lines in assets/js/server.js with two shorter ones.

This PR is based on previous not-yet-merged about typescript definitions and split into corresponding four commits for git history readability.

woutdp commented 11 months ago

These are all great thank you, exporting live instead of pushEvent/pushEventTo is a lot better.

Let me wrap my head around the changes for a little bit, I don't want to rush breaking changes, but will highly likely merge these during this week.

If you have more changes planned let me know so I'll wait a bit so I can do everything in one go :)

vonagam commented 11 months ago

Nothing planned much. But yeah, no hurry, take time.

For record just now I experimented with adding a preprocess to add export let live = undefined to a script if it does not contain it, simple to do but it leads to warning about an unused variable, so I guess no way to silence that thing.

vonagam commented 11 months ago

I looked into updating svelte version in package json from 3 to 4. There is an import in hooks from svelte/internal - import {detach, insert, noop} from "svelte/internal" and unfortunately esbuild does not tree shake it properly, even now it generates around 100 lines of unneeded code (out of 260 total). If version is updated to 4th one then that number grows. So I removed that import by copying those functions, thankfully those are basic one-liners.

Question: Why priv/static not in gitignore? Usually build files are put into ignore.

vonagam commented 11 months ago

So, actually preprocess does work, append \n;export let live = undefined; live = live; and it solves the warnings. But will need to add dependency on magic-string, this library already in dependencies of svelte 4 and used in all examples.

Marked it as external for esm/cjs build. But not for cdn/cdn_min because I am not sure if it is ok to do so. Does cdn support import statements or does it assume that everything was bundled in?

woutdp commented 10 months ago

Looked at it just now, I've merged some of the more simple and straightforward commits, feel free to rebase on master, it should make this PR a little bit smaller already.

There's still a couple of things I'm not sure about, let me go over them and we can handle them one by one. Probably most changes are ok to go, I just want to make sure I understand them correctly.

- pushEvent/pushEventTo -> live change

Unless user exports both pushEvent and pushEventTo svelte complains about passing of unknown props in dev mode

I don't see this error, I have dev: !deploy set. How do I reproduce it? The added benefit of including the other exported functions still stands though.

- Replace render/exportSvelteComponents with getRender This is fine, but would it be possible to split up the commit into 2, the refactor and the getRender change? It's hard for me to see which things changed because of getRender and which things changed because of the refactor.

- The preprocess commit https://github.com/woutdp/live_svelte/pull/65/commits/a2ab7eea3c8885a275cd346d5e6d97c125b3c4bd

Will this work if you do something like this?

export let someVariable, live

It feels brittle to me to fix this with a preprocesser, the above case is not handled I think.

vonagam commented 10 months ago

Ok, I rebased, split last commit into two - about render and about refactor in hooks, removed preprocessor commit.

I don't see this error, I have dev: !deploy set.

Strange, as you can see from linked code unless $$props or $$restProps are used there should be a warning for an unknown prop in dev mode. And I do get it, for example by going to example number 1, /simple.

Removed preprocessor for now, yeah it is not perfect, I don't think you can make an ideal one, there are tradeoffs to be made. It is an optional thing anyway, so can be forgot about for now.

woutdp commented 10 months ago

Ok nvm I see the error now.

Thanks for the quick response, I'll take a look at everything again now!

woutdp commented 10 months ago

Merged the changes, thanks for the great work :)

woutdp commented 10 months ago

Available in 0.11.0