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

Fix $app/paths import not working at dev mode inside context="module" #2145

Closed
wants to merge 8 commits into from
Closed

Fix $app/paths import not working at dev mode inside context="module" #2145

wants to merge 8 commits into from

Conversation

istarkov
Copy link
Contributor

@istarkov istarkov commented Aug 9, 2021

ref #2108

I've started playing with #2108 and found that

<script context="module">
import { base, assets } from '$app/paths';
...

Always return default values at dev and preview at serverside.

This is attempt to fix above.

Steps.

  • add failing test (created standalone for now as setting paths.base at options etc breaks everything)
  • add fix

Without above its hard to proceed with full paths.base support. As { base } import at module level would be needed for tests.

@changeset-bot
Copy link

changeset-bot bot commented Aug 9, 2021

⚠️ No Changeset found

Latest commit: 7cb7845

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@benmccann benmccann marked this pull request as draft August 9, 2021 17:38
@istarkov

This comment has been minimized.

@istarkov istarkov marked this pull request as ready for review August 9, 2021 19:13
@istarkov
Copy link
Contributor Author

ps: locally atop of this PR, I have fix for #2108 in dev and preview modes

In short I changed manifest routes pattern generation to include basePath, and cleared few places where basePath were spliced from pathname.

Even such solution doesnt allow dynamic basePath now, but it can be extended to be dynamic by changing pattern match logic a little bit.

/** @type {import('@sveltejs/kit').Load} */
export async function load({ page, fetch }) {
const res = await fetch('/host.json');
const res = await fetch(`${base}/host.json`);
Copy link
Member

Choose a reason for hiding this comment

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

Is this something you're going to want to do for every URL within the app or should we add a base html tag or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

btw I havent knew about base tag. :-) so probably its much better.

Would check tomorrow

@Rich-Harris
Copy link
Member

Thank you — the current behaviour was semi-intentional, but a little bit confusing (and error-prone, since you could easily forget to prefix URLs with base or assets), and this is a good opportunity to clarify it.

I think the base stuff makes sense. For assets it's a little trickier — a common use case for this option is to have your static assets hosted on a CDN far from your actual app, and as such it doesn't really make sense in dev or preview (since up-to-date assets won't have been uploaded to the CDN, and since it prevents things like offline development).

So I wonder if it makes sense to have a fake assets path during dev and preview? I wrote this up in #326 but then never got round to doing anything about it. We'd probably need to avoid changing req.url, since IIUC static_handler would respond to requests for /photo.jpg as well as ${assets}/photo.jpg, which we want to avoid.

@istarkov
Copy link
Contributor Author

Probably one of options would be to simplify a little bit.

In case of relative assets what the use case for asset to be not equal to base ?

For example our use case is PR deployments, because of 3rd party lib referer restrictions (no wildcards supported etc) we can't deploy each PR on own sub domain. So what we really need that same code work on any base path depending on env variable.

With our usecase we can set <base href="//{host}/{base}"> (thanks @benmccann I totally forgot about it) and everything like <a href='/blabla' or <img src="/blabla" would work.

If someone need different path for assets seems like changing appDir or/and adding subfolder into static will be enough.
As have different assets dir for different base makes code much more complicated.

So probably assets config must accept only non relative urls with schema defined i.e https://blabla, in all other cases assets must be same as base url.

At least for our usecase no need to do something like <img src="{asset}/blabla"/> and no need in fake path (as base tag would be enough).

In case of non relative assets fake path can be an option.

@Rich-Harris
Copy link
Member

In case of relative assets what the use case for asset to be not equal to base ?

🤯 this is... a great point. I don't think there is a good use case. We should probably just prohibit relative asset paths and use a fake one during development and preview. I have a bit of time today so will work on this now.

I've found <base> to be a pain in the neck, so in the long run I'd love for us to support dynamic basepaths. There's an issue for that — #595 — but it can be addressed separately from this

@Rich-Harris Rich-Harris mentioned this pull request Aug 13, 2021
5 tasks
@Rich-Harris
Copy link
Member

closing in favour of #2189 — thanks

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.

4 participants