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

Upgrade to Svelte 5 #476

Closed
wants to merge 104 commits into from
Closed

Upgrade to Svelte 5 #476

wants to merge 104 commits into from

Conversation

josdejong
Copy link
Owner

@josdejong josdejong commented Aug 1, 2024

Issues when upgrading to Svelte 5

  • Solve the Rollup bundle being broken. The build succeeds but when actually loading the standalone.js editor in a browser we get an error: "TypeError: props is undefined"

    Uncaught TypeError: props is undefined
    	prop props.js:226
    	JSONEditor JSONEditor.svelte:60
    	<anonymous> develop-vanilla.html:198
    

    Which is erroring at the first loaded component at the first line where a property is created:

     export let content: Content = { text: '' }
     // code from svelte
     export function prop(props, key, flags, fallback) {
     	var immutable = (flags & PROPS_IS_IMMUTABLE) !== 0;
     	var runes = (flags & PROPS_IS_RUNES) !== 0;
     	var lazy = (flags & PROPS_IS_LAZY_INITIAL) !== 0;
    
     	var prop_value = /** @type {V} */ (props[key]);    // <-- error at this line
     	var setter = get_descriptor(props, key)?.set;
  • Update documentation and examples on the API to create an editor: createJSONEditor(...) instead of new JSONEditor(...)

  • Refactor updateProps(...) so it doesn't use the deprecated this.$set(...)

  • Get rid of the need to configure compatibility: { componentApi: 4 }

  • Solve the circular dependency issues reported when bundling using Rollup (an issue in Svelte?)

    (!) Circular dependencies
    node_modules/svelte/src/internal/client/reactivity/effects.js -> node_modules/svelte/src/internal/client/runtime.js -> node_modules/svelte/src/internal/client/reactivity/effects.js
    node_modules/svelte/src/internal/client/reactivity/effects.js -> node_modules/svelte/src/internal/client/runtime.js -> node_modules/svelte/src/internal/client/dev/ownership.js -> node_modules/svelte/src/internal/client/reactivity/effects.js
    node_modules/svelte/src/internal/client/runtime.js -> node_modules/svelte/src/internal/client/dev/ownership.js -> node_modules/svelte/src/internal/client/runtime.js
    node_modules/svelte/src/internal/client/runtime.js -> node_modules/svelte/src/internal/client/reactivity/sources.js -> node_modules/svelte/src/internal/client/runtime.js
    node_modules/svelte/src/internal/client/runtime.js -> node_modules/svelte/src/internal/client/reactivity/deriveds.js -> node_modules/svelte/src/internal/client/runtime.js
    
  • All dependencies must support Svelte 5. Current dependencies with issues:

  • Fix the UI unit tests not working (try npm test): "Svelte error: lifecycle_function_unavailable mount(...) is not available on the server"

  • Fix issues with event handlers: cannot edit a value by double-clicking it. Double clicking a key works. (tree and table mode)

  • Fix styling issues: styling of selection and hovering background does not work when hovering an expanded array or object, or when selecting a number of properties in an object (tree mode). Solve all the Unused CSS selector compiler warnings.

  • Fix all compiler warnings

  • Remove deprecated Svelte option <svelte:options immutable={true} />

  • Replace occurrences of <div /> with <div></div>

To do's:

  • Convert the code to runes
  • Do a thorough manual testing round
  • Test for any performance regressions
  • Test using the library in a Svelte 4, Svelte 5, vanilla, and React (TS) project
  • Replace rollup scripts with Vite scripts?

josdejong added 30 commits May 6, 2024 15:51
…ctures (#450)

BREAKING CHANGE: 

Though the editor functions exactly the same, this refactor changes the internal core data structures, so be careful.
BREAKING CHANGE:

Changed the API to consistently use `undefined` instead of `null`. This involves properties `selection`, `onChange` (properties `contentErrors and `patchResult`), `onRenderContextMenu` (property `selection`), `onSelect`, and methods `validate`, and `select`.
BREAKING CHANGE:
Old deprecation messages are removed.
BREAKING CHANGE:

The API of the `expand` function is changed from `expand(callback)` to `expand(path, callback)`, 
and can't be used anymore for collapsing nodes. Instead, use the `collapse(path)` method for that.
# Conflicts:
#	package-lock.json
#	package.json
@dominikg
Copy link

dominikg commented Aug 1, 2024

fyi svelte-hmr isn't used with svelte5, its builtin now (compilerOptions.hmr).

@josdejong josdejong mentioned this pull request Aug 1, 2024
@josdejong
Copy link
Owner Author

Ah, you're right, thanks. I've removed my local node_modules and package-lock.json and run npm i --force again, now svelte-hmr doesn't popup again so I guess it was a left over from the Svelte 4 dependencies.

@dominikg
Copy link

dominikg commented Aug 1, 2024

rollup-plugin-svelte should work with svelte5 too, it has >=3 in peer and the implementation checks for .svelte.js files too and compiles them with compileModule

@dominikg
Copy link

dominikg commented Aug 1, 2024

if you encounter errors with rollup-plugin-svelte and svelte5, a report with a minimal reproduction to it's repo would help us.

@josdejong
Copy link
Owner Author

Yes I will try that.

@josdejong
Copy link
Owner Author

Ok @dominikg here you go: I created a new, empty Svelte project using npm create svelte@latest, upgraded it to Svelte 5, added a minimal component and Rollup config. The steps to reproduce are described in the README:

https://github.com/josdejong/test-svelte5-rollup

I hope this helps pinpointing the issue.

@dominikg
Copy link

dominikg commented Aug 1, 2024

i'm not sure i follow why you want to build with rollup like that. If you are publishing a svelte library, i suggest you take a look at svelte-package.

For your reproduction, please note that in svelte5, components are not classes anymore by default:
https://svelte-5-preview.vercel.app/docs/breaking-changes#components-are-no-longer-classes (although there is a compiler option to opt in to support)

you'd also have to bundle the svelte runtime, for it to work, but this is something i would not recommend doing.

@josdejong
Copy link
Owner Author

i'm not sure i follow why you want to build with rollup like that. If you are publishing a svelte library, i suggest you take a look at svelte-package.

I publish two versions of this library:

  1. svelte-jsoneditor for use in Svelte, using svelte-package and everyting.
  2. vanilla-jsoneditor for use in vanilla JS, Vue, React, Angular, etc. Here, I use Rollup to create a standalone bundle.

For your reproduction, please note that in svelte5, components are not classes anymore by default

Ahhh I get it, thanks for the pointer. I had seen these docs but for some reason it didn't land before 😅. The bundle works after using mount(Test, ...) instead of new Test(...) and adding a CSS processor, see this commit. I'll think through what API to offer to the users of the vanilla bundle, some factory function I think.

@dominikg do you think Svelte can fix the circular dependency issues reported by Rollup?

(!) Circular dependencies
node_modules/svelte/src/internal/client/reactivity/effects.js -> node_modules/svelte/src/internal/client/runtime.js -> node_modules/svelte/src/internal/client/reactivity/effects.js
node_modules/svelte/src/internal/client/reactivity/effects.js -> node_modules/svelte/src/internal/client/runtime.js -> node_modules/svelte/src/internal/client/dev/ownership.js -> node_modules/svelte/src/internal/client/reactivity/effects.js
node_modules/svelte/src/internal/client/runtime.js -> node_modules/svelte/src/internal/client/dev/ownership.js -> node_modules/svelte/src/internal/client/runtime.js
node_modules/svelte/src/internal/client/runtime.js -> node_modules/svelte/src/internal/client/reactivity/sources.js -> node_modules/svelte/src/internal/client/runtime.js
node_modules/svelte/src/internal/client/runtime.js -> node_modules/svelte/src/internal/client/reactivity/deriveds.js -> node_modules/svelte/src/internal/client/runtime.js

@dominikg
Copy link

dominikg commented Aug 2, 2024

instead of exposing mount/unmount or even the svelte component to consumers i'd recommend exporting a createInstance(targetSelector,props) function that returns a destroy function. That way vanilla consumers don't have to interact with svelte at all and you are free to change the internals in case a different version of svelte does it differently yet again.

@josdejong
Copy link
Owner Author

Thanks for the suggestion. I cannot find this createInstance though (not exposed in svelte or svelte/legacy or anything as far as I can see). But I expect I cannot use it since I want to return an instance of the editor with methods that can be used by vanilla users. Methods like .select(), .expand(...), .collapse(...), .scrollTo(...) etc, so just returning a destroy function is not enough.

I was thinking about this approach right now:

// factory function
export function createJSONEditor({ target, props }: Parameters<typeof mount>[1]) {
  return mount(JSONEditor, { target, props })
}

// and in the JSONEditor.svelte component: 
export async function destroy() {
  unmount(this)

  await tick() // await destroying
}

Not the most beautiful but it works 😅

@dominikg
Copy link

dominikg commented Aug 2, 2024

that destroy function is creative/clever. I was suggesting you make the factory function yourself, so what you posted is pretty much what i meant 👍

@josdejong
Copy link
Owner Author

😂 OK 👍

Thanks for the help! I'm really happy that I the library is mostly working now, I expect I can figure out the issues related to styling and event handling myself.

@dominikg
Copy link

dominikg commented Aug 2, 2024

great, happy svelting

thumb up and subscribe here for the circular dependencies sveltejs/svelte#10140

@benmccann
Copy link

I would suggest breaking this PR up into smaller parts. Quite a lot of these changes like removing <svelte:options immutable={true} /> and changing <td /> to <td></td> are changes that would be compatible with Svelte 4 and that could be merged without addressing all of the other issues. That would make it a bit easier to manage, to avoid merge conflict, see what is working vs still in progress, etc.

All dependencies must support Svelte 5. Current dependencies with issues:
[email protected]

I was using svelte-awesome in my site for a long time without issue

Replace rollup scripts with Vite scripts?

I generally prefer rollup to Vite's library mode. Svelte package might be worth checking out. But probably best to avoid trying to bite off even more as part of this PR

@josdejong
Copy link
Owner Author

Thanks for your feedback Ben (and good luck with finishing svelte 5.0, I see it's close!).

I may indeed use this PR just as experiment to explore what fixes/changes have to be done if there are a lot of merge conflicts.

Base automatically changed from v1 to main September 24, 2024 11:53
@josdejong
Copy link
Owner Author

Closing this PR in favor of #490, where as many as issues as possible are solved directly in the main branch.

@josdejong josdejong closed this Oct 15, 2024
@josdejong josdejong deleted the feat/svelte5 branch October 24, 2024 07:05
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.

3 participants