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: handle trailing slash on the image endpoint #12022

Merged
merged 5 commits into from
Sep 19, 2024

Conversation

Princesseuh
Copy link
Member

@Princesseuh Princesseuh commented Sep 18, 2024

Changes

Add a trailing slash to the image endpoint based on the trailing slash config, like we do for base

Fixes #11568

Testing

Added tests

Docs

N/A

Copy link

changeset-bot bot commented Sep 18, 2024

🦋 Changeset detected

Latest commit: 8579d22

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

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

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Sep 18, 2024
@Princesseuh Princesseuh marked this pull request as draft September 18, 2024 14:35
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Instead of adding import.meta.env.*. Maybe we can add the trailing slash to endpoint.route directly here?

.transform((config) => {
// If the user changed `outDir`, we need to also update `build.client` and `build.server`
// the be based on the correct `outDir`
if (
config.outDir.toString() !==
resolveDirAsUrl(ASTRO_CONFIG_DEFAULTS.outDir, fileProtocolRoot).toString()
) {
const outDirPath = fileURLToPath(config.outDir);
config.build.client = resolveDirAsUrl(originalBuildClient, outDirPath);
config.build.server = resolveDirAsUrl(originalBuildServer, outDirPath);
}
// Handle `base` trailing slash based on `trailingSlash` config
if (config.trailingSlash === 'never') {
config.base = prependForwardSlash(removeTrailingForwardSlash(config.base));
} else if (config.trailingSlash === 'always') {
config.base = prependForwardSlash(appendForwardSlash(config.base));
} else {
config.base = prependForwardSlash(config.base);
}
return config;
})

@Princesseuh
Copy link
Member Author

Instead of adding import.meta.env.*. Maybe we can add the trailing slash to endpoint.route directly here?

.transform((config) => {
// If the user changed `outDir`, we need to also update `build.client` and `build.server`
// the be based on the correct `outDir`
if (
config.outDir.toString() !==
resolveDirAsUrl(ASTRO_CONFIG_DEFAULTS.outDir, fileProtocolRoot).toString()
) {
const outDirPath = fileURLToPath(config.outDir);
config.build.client = resolveDirAsUrl(originalBuildClient, outDirPath);
config.build.server = resolveDirAsUrl(originalBuildServer, outDirPath);
}
// Handle `base` trailing slash based on `trailingSlash` config
if (config.trailingSlash === 'never') {
config.base = prependForwardSlash(removeTrailingForwardSlash(config.base));
} else if (config.trailingSlash === 'always') {
config.base = prependForwardSlash(appendForwardSlash(config.base));
} else {
config.base = prependForwardSlash(config.base);
}
return config;
})

I thought about it, but I figured that this value is generally useful to users when building URLs (much like BASE_URL or ASSET_PREFIX is) so we might as well expose it, what do you think?

@bluwy
Copy link
Member

bluwy commented Sep 19, 2024

I think there's value in it too, but for now it might collide with my plan to make the trailingSlash config an object (which isn't set in stone, but if it goes through, this feature would make it a little bumpy).

So I personally would hold off for a bit, but even if this goes through is not a very big deal 😅

@Princesseuh
Copy link
Member Author

Okay, I'll adjust and we can kick the can of "how to access config values in Astro" even further down the road 🫡

@Princesseuh Princesseuh marked this pull request as ready for review September 19, 2024 14:32
@Princesseuh Princesseuh changed the title feat: add trailing slash to import.meta.env for URLs building fix: handle trailing slash on the image endpoint Sep 19, 2024
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Why does the PR point next and not main?

@Princesseuh
Copy link
Member Author

Princesseuh commented Sep 19, 2024

Why does the PR point next and not main?

you can't customize the endpoint route in main, so this is not relevant. You'd have to fix the issue completely differently, but the issue on main is more niche than it is now on next

@Princesseuh Princesseuh merged commit ddc3a08 into next Sep 19, 2024
13 checks passed
@Princesseuh Princesseuh deleted the fix/endpoint-trailling-slash branch September 19, 2024 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants