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

Some breaking changes for 0.11 #65

Closed
wants to merge 3 commits into from

Conversation

vonagam
Copy link
Contributor

@vonagam vonagam commented Aug 1, 2023

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
Copy link
Owner

woutdp commented Aug 1, 2023

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
Copy link
Contributor Author

vonagam commented Aug 1, 2023

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
Copy link
Contributor Author

vonagam commented Aug 2, 2023

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
Copy link
Contributor Author

vonagam commented Aug 2, 2023

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
Copy link
Owner

woutdp commented Aug 8, 2023

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 a2ab7ee

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
Copy link
Contributor Author

vonagam commented Aug 8, 2023

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
Copy link
Owner

woutdp commented Aug 8, 2023

Ok nvm I see the error now.

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

@woutdp
Copy link
Owner

woutdp commented Aug 8, 2023

Merged the changes, thanks for the great work :)

@woutdp woutdp closed this Aug 8, 2023
@woutdp
Copy link
Owner

woutdp commented Aug 8, 2023

Available in 0.11.0

@vonagam vonagam deleted the breakish-changes branch August 8, 2023 19:48
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 this pull request may close these issues.

None yet

2 participants