Migrate extension webviews to Svelte 5#815
Migrate extension webviews to Svelte 5#815nobody5050 wants to merge 9 commits intowpilibsuite:2027from
Conversation
- Help - Project Creator - Project Importer - Dependency View
nobody5050
changed the title
|
There was a problem hiding this comment.
I may or may not review this more closely later, but just some broad details I'm noticing (and some of the recommendations may be wrong since I completed the Basic Svelte tutorial like 30 minutes ago):
- Almost all props seem to be optional, but that doesn't actually seem to be the case in practice; they're either defaulted in the child component or initialized with a value in the parent. In fact, several type definitions have their optional declaration erased because of that. How many props can we change to not be optional?
- Speaking of props, we're passing an insane number of props to several child components. Can some of this be replaced with global state in .svelte.ts files? Global state is kinda icky, but these webviews are small, so globals aren't used in very far away places.
- Let's try to completely dump Webpack in this PR. I'm not interested in dealing with multiple build systems.
- Can we make the config files use ES modules?
- There's a lot of functions without JSDoc. Try to add some API docs to newly added shared functions/components.
- Can we reduce the main.ts boilerplate by inlining as much as possible?
There was a problem hiding this comment.
Only used in RioLogEntry? Define it there instead.
| {/if} | ||
| </div> | ||
| </div> | ||
| {:else} |
There was a problem hiding this comment.
That is a seriously impressive amount of indentation (8 indents!) There has to be a way to simplify that...
| const svelte = require('rollup-plugin-svelte'); | ||
| const svelteConfig = require('./svelte.config'); | ||
|
|
||
| const production = process.env.NODE_ENV === 'production'; |
There was a problem hiding this comment.
This feels overkill to migrate. It's much leaner this way, and it's self contained.
There was a problem hiding this comment.
Going to push back on this one. I'd rather have all webviews be done with svelte than a mix, especially if we ever want to expand the help page in the future
There was a problem hiding this comment.
Maybe, but I’m still not a fan of shipping 200 kB of JavaScript just for that page.
There was a problem hiding this comment.
Because svelte doesn't use a virtual DOM and instead compiles, the actual cost of shipping this should be negligible compared to the old way. See: https://svelte.dev/blog/virtual-dom-is-pure-overhead
| "sourceMap": true, | ||
| "rootDir": "src", | ||
| "moduleResolution": "node", | ||
| "allowSyntheticDefaultImports": true, |
| $effect(() => { | ||
| projectFolderError = validateProjectFolder(projectFolder); | ||
| }); | ||
| $effect(() => { | ||
| projectNameError = validateProjectName(projectName); | ||
| }); | ||
| $effect(() => { | ||
| teamNumberError = validateTeamNumber(teamNumber); | ||
| }); |
There was a problem hiding this comment.
Does this replace localeloader.ts?
There was a problem hiding this comment.
Why function here compared to arrow functions everywhere else?
| name: 'localeloader', | ||
| input: 'src/webviews/localeloader.ts', | ||
| title: 'Locale Loader', |
There was a problem hiding this comment.
This is a library, not a webview.
There was a problem hiding this comment.
Maybe, but I’m still not a fan of shipping 200 kB of JavaScript just for that page.
4 participants
This PR converts the webviews to a frontend framework, namely Svelte 5. All of the webviews are now built with rollup and share a lot more infrastructure, so i18n and similar should be easier to maintain. They also have the benefit of sharing components so the different wizards are easier to keep in sync.
Closes #55
Status: