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

resolvePath output should be prefixed with kit.paths.base #10282

Closed
hmnd opened this issue Jun 29, 2023 · 5 comments
Closed

resolvePath output should be prefixed with kit.paths.base #10282

hmnd opened this issue Jun 29, 2023 · 5 comments
Assignees
Labels
breaking change feature / enhancement New feature or request needs-decision Not sure if we want to do this yet, also design work needed paths.base bugs relating to `config.kit.paths.base`
Milestone

Comments

@hmnd
Copy link
Contributor

hmnd commented Jun 29, 2023

Describe the bug

The current output of resolvePath is not prefixed with the app's base path, making it a hassle to use with a custom base.

Reproduction

  1. Set custom kit.paths.base in svelte.config.js
  2. Call resolvePath()

Logs

No response

System Info

System:
    OS: Linux 6.3 Manjaro Linux
    CPU: (12) x64 Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
    Memory: 5.60 GB / 15.50 GB
    Container: Yes
    Shell: 5.9 - /usr/bin/zsh
  Binaries:
    Node: 18.16.0 - ~/.asdf/installs/nodejs/lts/bin/node
    npm: 9.5.1 - ~/.asdf/plugins/nodejs/shims/npm
    pnpm: 8.6.4 - ~/.local/share/pnpm/pnpm
  Browsers:
    Brave Browser: 114.1.52.129
  npmPackages:
    @sveltejs/adapter-auto: ^2.1.0 => 2.1.0 
    @sveltejs/adapter-cloudflare-workers: ^1.1.2 => 1.1.2 
    @sveltejs/kit: ^1.20.0 => 1.20.4 
    svelte: 4.0.0 => 4.0.0 
    vite: ^4.3.9 => 4.3.9

Severity

annoyance

Additional Information

I'm aware this would introduce a technically breaking change, so perhaps this should be exported as a separate function, or added as a flags to resolvePath to preserve current behavior.

@gtm-nayan gtm-nayan added feature / enhancement New feature or request breaking change paths.base bugs relating to `config.kit.paths.base` labels Jun 29, 2023
@benmccann benmccann added the needs-decision Not sure if we want to do this yet, also design work needed label Dec 5, 2023
@benmccann benmccann added this to the 2.0 milestone Dec 5, 2023
@dummdidumm
Copy link
Member

We can't take kit.paths.base into account automatically, because resolvePath is part of exports/index.js, which can't integrate with the generated stuff. I see two options:

  • add resolvePath to $app/navigation or $app/paths, deprecate resolvePath from @sveltejs/kit and remove it in version 3
  • add a third optional parameter named prefix to resolvePath, to which you can pass the base path (or anything else, if needed)

I don't know which one I like better yet, I guess it depends on how many use cases there are to use resolvePath without wanting kit.paths.base prepended.

@dummdidumm
Copy link
Member

We're making this part of $app/paths or $app/navigation, which one we're not totally sure about yet.

@hmnd
Copy link
Contributor Author

hmnd commented Dec 9, 2023

I would lean slightly toward putting it in $app/navigation, since

  • $app/paths only provides static values
  • resolvePath is (probably?) often used in tandem with goto

@benmccann
Copy link
Member

I wasn't even aware this API existed. I might take this opportunity to rename it to resolveRoute. E.g. if you go from a domain name to an IP address, I think you call it resolving the domain name and not resolving the IP address. In this case we're going from route to path, so we should call it resolving the route.

Another option would be to put it in $app/routes. If we want to reuse an existing module then I'd probably lean more towards $app/paths since we're getting a path and we're not navigating.

@Rich-Harris
Copy link
Member

It should definitely be $app/paths. I don't have a strong opinion on naming but resolveRoute seems fine

@Rich-Harris Rich-Harris self-assigned this Dec 11, 2023
dummdidumm added a commit that referenced this issue Dec 12, 2023
closes #10282

---------

Co-authored-by: Rich Harris <rich.harris@vercel.com>
Co-authored-by: Simon Holthausen <simon.holthausen@vercel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change feature / enhancement New feature or request needs-decision Not sure if we want to do this yet, also design work needed paths.base bugs relating to `config.kit.paths.base`
Projects
None yet
Development

No branches or pull requests

5 participants