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

feat: add editor label and formatter configuration defaults #2330

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

eltigerchino
Copy link
Member

@eltigerchino eltigerchino commented Apr 5, 2024

Uses the VS Code extension configuration defaults contribution point to:

  • set the default formatter for svelte files as mentioned in the README (which some people may have missed)
  • uses the new VS Code custom labels setting to append the directory name to the editor label for SvelteKit +page and +server files.

Default

default editor labels

Label format

"workbench.editor.labelFormat": "short"

label format: short

Custom labels

  "workbench.editor.customLabels.patterns": {
    "**/+page.*": "${dirname}/${filename}.${extname}",
    "**/+server.*": "${dirname}/${filename}.${extname}",
  },

custom vscode editor labels

@benmccann
Copy link
Member

I wonder if we could drop routes from the example. It makes sense to include the directory for URLs like /about, but we don't necessarily need it for the root directory where we could show just /+page.js. Maybe we can have another entry earlier without the leading **?

I also wonder if we could show /about/+page.js if it's only one level deep and .../[id]/+page.js if it's multiple levels deep

@eltigerchino
Copy link
Member Author

eltigerchino commented Apr 5, 2024

I wonder if we could drop routes from the example. It makes sense to include the directory for URLs like /about, but we don't necessarily need it for the root directory where we could show just /+page.js. Maybe we can have another entry earlier without the leading **?

I had this initially too but realised it could break if anyone changed the files.routes config and it no longer points to src/routes. But this should be a minority of users? https://kit.svelte.dev/docs/configuration#files

I also wonder if we could show /about/+page.js if it's only one level deep and .../[id]/+page.js if it's multiple levels deep

Maybe something like this?

Details
"workbench.editor.customLabels.patterns": {
    "src/routes/+[page|server]*": "/${filename}.${extname}",
    "src/routes/*/+[page|server]*": "/${dirname}/${filename}.${extname}",
    "src/routes/*/*/**/+[page|server]*": ".../${dirname}/${filename}.${extname}",
},

label variants based on nesting

@@ -396,6 +396,16 @@
}
}
},
"configurationDefaults": {
"[svelte]": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure we want this because people can also use the prettier extension, if that ones installed I'd actually prefer that one was used assuming people have both prettier and the plugin installed.
The label thing is a super nice addition.

Copy link
Member Author

@eltigerchino eltigerchino Apr 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, but what's the difference between the two? I've been using the svelte formatter set as the default although I have both installed. Mainly because setting prettier as the default formatter prompts me to change the default when I try to format a svelte file in a project that has no prettier installed or prettier config.

@benmccann
Copy link
Member

I had this initially too but realised it could break if anyone changed the files.routes config and it no longer points to src/routes. But this should be a minority of users? https://kit.svelte.dev/docs/configuration#files

We could potentially duplicate the rules and remove the src/routes from the second set as a fallback for users who have changed that config setting

@eltigerchino
Copy link
Member Author

I had this initially too but realised it could break if anyone changed the files.routes config and it no longer points to src/routes. But this should be a minority of users? kit.svelte.dev/docs/configuration#files

We could potentially duplicate the rules and remove the src/routes from the second set as a fallback for users who have changed that config setting

I'm not sure how to get the fallback rules to recognise the nesting level since without knowing the files.routes configuration. I did add a very general fallback rule to just append the directory name which isn't as nice but maybe it's better than nothing

@eltigerchino
Copy link
Member Author

eltigerchino commented Apr 6, 2024

Another suggestion regarding layout groups:

If you had layout groups, your tabs wouldn't give you the context of what route the file if for:
layout groups only as tab labels because they are the immediate directory

So maybe we can show the directory before the layout group?

layout groups and the directory before it as tab labels

"src/routes/(*)/+[layout|page|server]*": "/${filename}.${extname} ${dirname}",
"src/routes/*/**/(*)/+[layout|page|server]*": "/${dirname(1)}/${filename}.${extname} ${dirname(0)}",

EDIT: actually this is a terrible idea since it would be inconsistent for deeper nested directories while we don't have capture groups

@paoloricciuti
Copy link
Member

I wonder if we could drop routes from the example. It makes sense to include the directory for URLs like /about, but we don't necessarily need it for the root directory where we could show just /+page.js. Maybe we can have another entry earlier without the leading **?

I also wonder if we could show /about/+page.js if it's only one level deep and .../[id]/+page.js if it's multiple levels deep

I tried exactly this in my configuration but removed it because:

  1. With very nested stuff it can get very long and it will still not reflect the actual route
  2. With not so nested stuff it will include routes which I didn't like
  3. In the end I got already used to the current version and I prefer being precise and not changing stuff and maybe make it more confusing

@ottomated
Copy link

ottomated commented Apr 16, 2024

This is what I came up with:

"workbench.editor.customLabels.patterns": {
	// Root
	"**/routes/+layout.svelte": "/ [layout]",
	"**/routes/+page.server.ts": "/ [server]",
	"**/routes/+layout.server.ts": "/ [layout.server]",
	"**/routes/+server.ts": "/ [endpoint]",
	"**/routes/+page.svelte": "/ [page]",
	// 1st level
	"**/routes/*/+layout.svelte": "/${dirname} [layout]",
	"**/routes/*/+page.server.ts": "/${dirname} [server]",
	"**/routes/*/+layout.server.ts": "/${dirname} [layout.server]",
	"**/routes/*/+server.ts": "/${dirname} [endpoint]",
	"**/routes/*/+page.svelte": "/${dirname} [page]",
	// Nested
	"**/routes/*/*/**/+layout.svelte": "${dirname(1)}/${dirname} [layout]", 
	"**/routes/*/*/**/+page.server.ts": "${dirname(1)}/${dirname} [server]",
	"**/routes/*/*/**/+layout.server.ts": "${dirname(1)}/${dirname} [layout.server]",
	"**/routes/*/*/**/+server.ts": "${dirname(1)}/${dirname} [endpoint]",
	"**/routes/*/*/**/+page.svelte": "${dirname(1)}/${dirname} [page]",
}

Very convoluted but this makes things like src/routes/+page.svelte -> / [page] but src/routes/api/nested/endpoint/+server.ts -> nested/endpoint [endpoint].

@benmccann
Copy link
Member

I tried exactly this in my configuration but removed it because:
With not so nested stuff it will include routes which I didn't like

@paoloricciuti I'm not sure you did try the same thing then. My suggestion was that routes should never be shown

@ottomated your setup can't differentiate between +page.svelte and +page.js

Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@ottomated
Copy link

ottomated commented Apr 17, 2024

@ottomated your setup can't differentiate between +page.svelte and +page.js

Good catch, here's a more comprehensive version:

"workbench.editor.customLabels.patterns": {
	// Root
	"**/routes/+layout.svelte": "/ [layout]",
	"**/routes/+layout.server.[jt]s": "/ [layout.server.load]",
	"**/routes/+layout.[jt]s": "/ [layout.load]",
	"**/routes/+page.svelte": "/ [page]",
	"**/routes/+page.server.[jt]s": "/ [server.load]",
	"**/routes/+page.[jt]s": "/ [load]",
	"**/routes/+server.[jt]s": "/ [endpoint]",
	// 1st level
	"**/routes/*/+layout.svelte": "/${dirname} [layout]",
	"**/routes/*/+layout.server.[jt]s": "/${dirname} [layout.server.load]",
	"**/routes/*/+layout.[jt]s": "/${dirname} [layout.load]",
	"**/routes/*/+page.svelte": "/${dirname} [page]",
	"**/routes/*/+page.server.[jt]s": "/${dirname} [server.load]",
	"**/routes/*/+page.[jt]s": "/${dirname} [load]",
	"**/routes/*/+server.[jt]s": "/${dirname} [endpoint]",
	// Nested
	"**/routes/*/*/**/+layout.svelte": "${dirname(1)}/${dirname} [layout]",
	"**/routes/*/*/**/+layout.server.[jt]s": "${dirname(1)}/${dirname} [layout.server.load]",
	"**/routes/*/*/**/+layout.[jt]s": "${dirname(1)}/${dirname} [layout.load]",
	"**/routes/*/*/**/+page.svelte": "${dirname(1)}/${dirname} [page]",
	"**/routes/*/*/**/+page.server.[jt]s": "${dirname(1)}/${dirname} [server.load]",
	"**/routes/*/*/**/+page.[jt]s": "${dirname(1)}/${dirname} [load]",
	"**/routes/*/*/**/+server.[jt]s": "${dirname(1)}/${dirname} [endpoint]",
},

@sacrosanctic
Copy link

what about +error.svelte?

triyuga pushed a commit to triyuga/sveltekit-5-demo-app that referenced this pull request Sep 16, 2024
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.

6 participants