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.

Sign up for GitHub

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

Jump to bottom

[Fix] broken assets path #6962

Closed
wants to merge 5 commits into from
Closed

[Fix] broken assets path #6962

wants to merge 5 commits into from

Conversation

frankmayer
Copy link

@frankmayer frankmayer commented Sep 22, 2022 •
edited
Loading

Fixes #6326

This fixes broken usage of imagetools and possibly other vite plugins as well - when asset paths are used with the static adapter.
The breaking PR was #4250.

This fix introduces a new config option rootRelativeAssets relativeAssets, which defaults to true false, restoring the way things worked prior to the breaking PR, => namely: non-relative asset paths (or a "root-based asset path").

EDIT: Changed option name and logic

This approach may or may not be the correct one and may be up for discussion or be rejected completely. For that reason, I have not written a test for that, yet. But I have tested the fix successfully on a project which suffered that issue.

If this is a viable fix, I'll gladly complete this PR with anything else needed.

...and thank you all for this amazing project 🥇

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@frankmayer
Fix root-relative assets for subpages
1006b9c 
Introduces a new option in svelte config: rootRelativeAssets (boolean), which defaults to true, in order to restore previous functionality as a default.
@frankmayer
Merge commit 'd0e63d445e19e2df7a5e7d35f42b2ba1ae5e5745'
d491abc
@changeset-bot
Copy link

changeset-bot bot commented Sep 22, 2022 •
edited
Loading

⚠️ No Changeset found

Latest commit: 18df33f

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 linked an issue Sep 22, 2022 that may be closed by this pull request
Relative asset paths broke usage of imagetools #6326
Closed
@frankmayer frankmayer changed the title Fix assets path (by choosing between root relative or path relative via option) Fix assets path (by choosing between root based or path relative via config option) Sep 23, 2022
@frankmayer
- rename rootRelativeAssets to relativeAssets
1f0404c 
- set `relativeAssets` default to false
- adjust logic
- adjustment of option placement in spec file
@frankmayer frankmayer changed the title Fix assets path (by choosing between root based or path relative via config option) [Fix] assets path (by choosing between root based or path relative via config option) Sep 28, 2022
@frankmayer frankmayer changed the title [Fix] assets path (by choosing between root based or path relative via config option) [Fix] broken assets path Sep 28, 2022
benmccann
benmccann requested changes Sep 28, 2022
View reviewed changes
Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

this doesn't seem like the right fix to me for a couple reasons

one is that we should make it work with relative assets

the other is that this seems to also be affecting laravel, so it's quite possibly an issue in vite-imagetools: JonasKruckenberg/imagetools#396

@benmccann
Copy link
Member

Can you check if JonasKruckenberg/imagetools#400 would fix the issue you're facing?

@frankmayer
Copy link
Author

Thank you for your input. You were right.
I tested JonasKruckenberg/imagetools#400 with adapter-static and it seems to work now.

@frankmayer frankmayer closed this Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benmccann benmccann benmccann requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Relative asset paths broke usage of imagetools
2 participants
@frankmayer @benmccann