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

Handle base in adapters #5290

Merged
merged 5 commits into from
Nov 7, 2022
Merged

Handle base in adapters #5290

merged 5 commits into from
Nov 7, 2022

Conversation

matthewp
Copy link
Contributor

@matthewp matthewp commented Nov 3, 2022

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 new removeBase 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.
  • For the 3 adapters that use this functionality they all have a major bump as they now depend on it via the peerDependency.
  • Fixes The config value for the base property is not working as expected in SSR mode #5288

Testing

Test added in our test adapter which mimics the scenario.

Docs

N/A, bug fix

@changeset-bot
Copy link

changeset-bot bot commented Nov 3, 2022

🦋 Changeset detected

Latest 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

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope) labels Nov 3, 2022
@matthewp matthewp marked this pull request as ready for review November 3, 2022 16:01
@matthewp matthewp requested a review from a team as a code owner November 3, 2022 16:01
@sarah11918 sarah11918 removed the request for review from a team November 3, 2022 21:14
@cauboy
Copy link

cauboy commented Nov 4, 2022

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 _worker.js file. The value of the links property in the shown item is ["assets/index.857d2ba4.css"] but should be imho ["my-astro-app/assets/index.857d2ba4.css"] – at least prefixing the value with my-astro-app/ results in the CSS being imported in the rendered HTML file.

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:

  • ✅ Is fixed with this PR

    ❌ Website is not accessible via http://localhost:port/my-astro-app/ but via http://localhost:port/

  • ✅ In the served HTML, the file paths of static assets e.g. images are prefixed with my-astro-app/
  • ❌ Is not fixed within this PR.
    The stylesheet is still linked via <link rel="stylesheet" href="/assets/index.857d2ba4.css"> in the rendered HTML – (Not sure if it was intentionally not addressed b/c it's out of scope of this PR)

    ❌ In the served HTML, the rel attributed of the linked stylesheet is not prefixed with my-astro-app/: Instead of it's

  • ✅ Is fixed with this PR (files still live in dist/static/ but are accessible via http://localhost:port/my-astro-app now

    ❌ All files are put to dist/static/* instead of dist/static/my-astro-app/*

Other Observations:

  1. When opening the url without trailing slash (http://localhost:port/my-astro-app instead of http://localhost:port/my-astro-app/) a 404 Not Found error is returned. I would expect both URLs to work – but might also be the wanted behavior

matthewp and others added 2 commits November 4, 2022 11:36
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
@matthewp
Copy link
Contributor Author

matthewp commented Nov 4, 2022

@cauboy I'm AFC until Monday, I'll try out your demo and address any issues with that in this PR.

@matthewp
Copy link
Contributor Author

matthewp commented Nov 4, 2022

Considering this PR blocked for now.

@cauboy
Copy link

cauboy commented Nov 7, 2022

One more remark:

In the implementation, static assets stay in the "root folder for static assets", e.g. static/* in Cloudflare. When a static asset is requested, it's then checked whether /${base}/${pathname} is listed in the assets section of the manifest and if so, the requested is forwarded to /${pathname} by creating another request , see here for Cloudflare's advanced mode and here for Cloudflare's directory mode.

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. static/my-astro-app/*.

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.

@matthewp
Copy link
Contributor Author

matthewp commented Nov 7, 2022

Thanks @cauboy, to your first reply I'm going to look into the <link href> issue that's not fixed. We should be prefixing those so I'll look into that now.

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.

@matthewp matthewp merged commit b2b291d into main Nov 7, 2022
@matthewp matthewp deleted the base-plus-ssr-plus-assets branch November 7, 2022 15:05
@astrobot-houston astrobot-houston mentioned this pull request Nov 7, 2022
@AirBorne04
Copy link
Contributor

In the implementation, static assets stay in the "root folder for static assets", e.g. static/* in Cloudflare. When a static asset is requested, it's then checked whether /${base}/${pathname} is listed in the assets section of the manifest and if so, the requested is forwarded to /${pathname} by creating another request , see here for Cloudflare's advanced mode and here for Cloudflare's directory mode.
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. static/my-astro-app/*.

I think the reason for this is that there was a worker (single file) implementation for assets in the past (utilizing KV for static assets).

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.

I do believe you are correct here, it is probably 2x Worker Invocation vs 0 (at best). Because the 1. request to static/* than the 2. new Request with adjusted path. In order to make it 0 we would need to do 2 things,

  1. rewrite the asset path to static (just in directory mode)
  2. create a _routes.json which excludes the static folder from the routing

@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.

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) pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The config value for the base property is not working as expected in SSR mode
4 participants