-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Handle base
in adapters
#5290
Handle base
in adapters
#5290
Conversation
🦋 Changeset detectedLatest commit: 3daad51 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 |
Hi, I reported #5288 and was checking out this PR. This PR fixes most of the issues I reported. But the issue addressing the linked CSS file in the rendered HTML remains. The following code snippet is taken from the generated Snippet of `_worker.js` file{
adapterName: "@astrojs/cloudflare",
routes: [
{
file: "",
links: ["assets/index.857d2ba4.css"],
scripts: [
{
type: "inline",
value: `document.toggleNav=o;function o(){const t=document.getElementById("NavigationBar.nav"),n=document.getElementById("NavigationBar.nav.button.icon");!t||!n||(t.classList.toggle("hidden"),n.classList.toggle("-rotate-90"),console.log())}
`,
},
],
routeData: {
route: "/",
type: "page",
pattern: "^\\/$",
segments: [],
params: [],
component: "src/pages/index.astro",
pathname: "/",
_meta: { trailingSlash: "ignore" },
},
},
// more … Here's my summary:
Other Observations:
|
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
@cauboy I'm AFC until Monday, I'll try out your demo and address any issues with that in this PR. |
Considering this PR blocked for now. |
One more remark: In the implementation, static assets stay in the "root folder for static assets", e.g. Is there a specific reason why it's done like that? Why aren't static assets moved directly to a base-prefixed folder, e.g. Hypothesis: Depending on the pricing model of the server/edge provider this could result in twice the costs as a worker invocation is required to serve a static asset instead of serving it directly from the static folder. |
Thanks @cauboy, to your first reply I'm going to look into the About the trailing slash behavior, this is controllable with trailing slash config. As for where files are moved, that is outside of my wheelhouse as I really don't know what's best here for Cloudflare. Would moving it to another subfolder mean that it never hits the worker? How does cloudflare know to look in that subfolder. I'd like to work on this in a follow-up PR, perhaps someone like @AirBorne04 (or yourself) who uses Cloudflare can tackle this part. |
I think the reason for this is that there was a
I do believe you are correct here, it is probably 2x Worker Invocation vs 0 (at best). Because the 1. request to
@cauboy At the moment you could work around the double call by moving the files yourself. I will pull something together for this in the coming days. |
Changes
src/app
now trims the base when looking up routes, so it will match routes when the URL includes the base.src/app
has a newremoveBase
method which trims the base from a pathname. This is useful for adapters that need to look up assets from the filesystem, such as the Node and Deno adapters.major
bump as they now depend on it via thepeerDependency
.Testing
Test added in our test adapter which mimics the scenario.
Docs
N/A, bug fix