Improvements to build and dev when building for subpaths #3156

tony-sull · 2022-04-20T12:37:43Z

Changes

Fixes a bug in astro build that was breaking injected links and styles in projects deployed to subpaths
[x] fixes a bug where astro build wasn't prepending the base path for injected links and styles
[x] updates the dev server to handle the base path when assets from public are requested

Testing

Updated static-build test suite to expect injected style links to include the base path from astro.config.js

Docs

N/A

changeset-bot · 2022-04-20T12:37:46Z

🦋 Changeset detected

Latest commit: c13a936

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

This PR includes changesets to release 1 package

Name	Type
astro	Patch

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

packages/astro/src/core/build/generate.ts

tony-sull · 2022-04-20T17:42:55Z

packages/astro/src/vite-plugin-astro-server/index.ts

@@ -111,6 +111,25 @@ async function handle404Response(
 if (pathname === '/' && !pathname.startsWith(devRoot)) {
 html = subpathNotUsedTemplate(devRoot, pathname);
 } else {
+ // HACK: redirect without the base path for assets in publicDir
+ const redirectTo = 


Not too happy with doing a redirect here...

We currently handle base paths ourselves and don't let Vite do it. In dev, that means Vite sends us a 404 on assets that are in public

Longer term it might be the right move to lean on Vite for base config.

I tried this locally hoping that would do the trick and the main hurdle is that Astro.request will never be given context on the base path since Vite strips that out. That kind of makes sense actually Astro.request.url would always match the src/pages directory structure, but would be a breaking change from current behavior

so as a general rule, we usually don't want to redirect in dev since the developer might accidentally rely on that behavior during dev, and then ship to prod with broken code. Not sure I 100% understand what's going on here so can't say for sure, just as long as you're accounting for the goal: "don't implement helpful behavior in dev that might not exist in prod. It's usually better to error."

Now having said that, we do default trailingSlash to ignore, so maybe developer convenience is also worth taking into account.

This is a bit of a weird case for sure!

Assets in public will hit a 404 in dev. public/images/penguin.png should be referenced as src="/subpath/images/penguin.png" but Vite will 404 there, this redirect will only redirect if the requests pathname matches /{subpath}/...

The redirect strips off the subpath and sends that as the 302 location, letting the browser find the image in public even though templates will use /subpath/... for the <img src>

packages/astro/test/static-build.test.js

natemoo-re · 2022-04-20T17:41:44Z

packages/astro/src/core/path.ts

+}
+
+export function joinPaths(...paths: (string | undefined)[]) {
+ return paths.filter(isString).map(trimSlashes).join('/');


I prefer being explicit and using filter(x => isString(x)) rather than filter(isString). Ditto with trimSlashes.

Typescript doesn't figure out type narrowing with inline functions 🤔

// TS recognizes that only strings pass the filter paths.filter(isString).map(trimSlashes) // TS ignores the filter type and errors that `trimSlashes` is being called with `string | undefined` paths.filter(x => isString(x)).map(x => trimSlashes(x))

yup, this is oddly the "TS-approved" way to do it, afaik

natemoo-re · 2022-04-20T17:42:36Z

packages/astro/src/core/build/generate.ts

+ const site = astroConfig.base && astroConfig.base !== './'
+ ? joinPaths(astroConfig.site?.toString() || 'http:https://localhost/', astroConfig.base)
+ : astroConfig.site;


This is the fix I expected. Are we checking that astroConfig.site doesn't also end with base? Maybe that's validated when we load the config.

Yep! We already throw an error if the site includes a URL path

[config] Astro found issue(s) with your configuration:
! site "site" must be a valid URL origin (ex: "https://example.com") but cannot contain a URL path (ex: "https://example.com/blog"). Use "base" to configure your deployed URL path.

natemoo-re · 2022-04-20T17:43:50Z

packages/astro/test/static-build.test.js

@@ -90,7 +94,7 @@ describe('Static build', () => {
 const links = $('link[rel=stylesheet]');
 for (const link of links) {
 const href = $(link).attr('href');
- const data = await fixture.readFile(addLeadingSlash(href));
+ const data = await fixture.readFile(removeBasePath(addLeadingSlash(href)));


Hmm... how does this work? Are the assets emitted at /subpath/assets or /assets?

Assets are built to /dist, the main astro build bug here was that injected scripts and links didn't include the subpath

This test finds a stylesheet similar to <link rel="stylesheet" href="/subpath/assets/chunk.css"> and strips off the subpath when verifying the file exists (mimicing the behavior once it's deployed to a subpath in production)

matthewp · 2022-04-21T17:03:46Z

Now that I understand the behavior of site/base better, this all makes sense to me. 👍

packages/astro/src/vite-plugin-astro-server/index.ts

…)" This reverts commit 637919c.

* Revert "Improvements to build and dev when building for subpaths (#3156)" This reverts commit 637919c. * letting Vite handle base paths * test updates to expect Astro.request.url to no longer include subpaths * bringing back the fix for including subpaths in injects scripts and styles * fixing the static-build test to handle subpaths for injected CSS * fixing asset import URLs when using base subpaths * chore: fixing typo in the comments * Astro needs to manage base in dev to maintain Astro.request.url * fix: reverting dev routing tests to expect existing behavior * reverting Astro global test to verify existing behavior * chore: adding changeset * test: update static-build tests to verify the subpath is used in asset imports

…tro#3178) * Revert "Improvements to build and dev when building for subpaths (withastro#3156)" This reverts commit 637919c. * letting Vite handle base paths * test updates to expect Astro.request.url to no longer include subpaths * bringing back the fix for including subpaths in injects scripts and styles * fixing the static-build test to handle subpaths for injected CSS * fixing asset import URLs when using base subpaths * chore: fixing typo in the comments * Astro needs to manage base in dev to maintain Astro.request.url * fix: reverting dev routing tests to expect existing behavior * reverting Astro global test to verify existing behavior * chore: adding changeset * test: update static-build tests to verify the subpath is used in asset imports

) * `astro build` should include the `base` provided in astro config * test: updating build test to expect the base path in links/scripts * ignore the default "base" value when building links/scripts * fix: handling config that provides a base but no site * fix: config.site was being ignored since it's a URL not a string * hack: handle base path in dev for /public assets * fix: dev redirect needs to ignore base default of ./ * fix: extra safety checks for the base path redirect * refactor: simplifying it to remove the regex * one last safety check - only redirect GET and use a 302 status * fix: lost the leading slash when redirecting * nit: adding comments to the test explaining how base is verified * Remove extra console.log * Adds a changeset Co-authored-by: unknown <[email protected]>

…tro#3178) * Revert "Improvements to build and dev when building for subpaths (withastro#3156)" This reverts commit 637919c. * letting Vite handle base paths * test updates to expect Astro.request.url to no longer include subpaths * bringing back the fix for including subpaths in injects scripts and styles * fixing the static-build test to handle subpaths for injected CSS * fixing asset import URLs when using base subpaths * chore: fixing typo in the comments * Astro needs to manage base in dev to maintain Astro.request.url * fix: reverting dev routing tests to expect existing behavior * reverting Astro global test to verify existing behavior * chore: adding changeset * test: update static-build tests to verify the subpath is used in asset imports

github-actions bot added pkg: astro Related to the core `astro` package (scope) test labels Apr 20, 2022

tony-sull marked this pull request as draft April 20, 2022 12:53

tony-sull force-pushed the fix/build-subpaths branch from 4a70a34 to 6adf1f1 Compare April 20, 2022 13:11

matthewp reviewed Apr 20, 2022

View reviewed changes

packages/astro/src/core/build/generate.ts Outdated Show resolved Hide resolved

tony-sull marked this pull request as ready for review April 20, 2022 15:23

Tony Sullivan added 10 commits April 20, 2022 17:24

astro build should include the base provided in astro config

66e2f56

test: updating build test to expect the base path in links/scripts

f72a3e5

ignore the default "base" value when building links/scripts

8d47bc4

fix: handling config that provides a base but no site

6567ddc

fix: config.site was being ignored since it's a URL not a string

cb958ae

hack: handle base path in dev for /public assets

c0c485a

fix: dev redirect needs to ignore base default of ./

5d11034

fix: extra safety checks for the base path redirect

84d5c73

refactor: simplifying it to remove the regex

9bf2a08

one last safety check - only redirect GET and use a 302 status

4ada547

tony-sull force-pushed the fix/build-subpaths branch from 614cde1 to 4ada547 Compare April 20, 2022 15:24

fix: lost the leading slash when redirecting

2b3e2bb

tony-sull changed the title ~~Include base path when building injected scripts and links~~ Improvements to build and dev when building for subpaths Apr 20, 2022

tony-sull commented Apr 20, 2022

View reviewed changes

natemoo-re reviewed Apr 20, 2022

View reviewed changes

nit: adding comments to the test explaining how base is verified

96e82fd

matthewp reviewed Apr 21, 2022

View reviewed changes

packages/astro/src/vite-plugin-astro-server/index.ts Outdated Show resolved Hide resolved

matthewp added 2 commits April 21, 2022 13:48

Remove extra console.log

ef1d874

Adds a changeset

c13a936

matthewp merged commit 637919c into main Apr 21, 2022

matthewp deleted the fix/build-subpaths branch April 21, 2022 18:03

github-actions bot mentioned this pull request Apr 21, 2022

[ci] release #3169

Merged

tony-sull pushed a commit that referenced this pull request Apr 22, 2022

Revert "Improvements to build and dev when building for subpaths (#3156…

d80b113

…)" This reverts commit 637919c.

tony-sull mentioned this pull request Apr 22, 2022

Allowing Vite to handle base config for deploying to subpaths #3178

Merged

Auroratide mentioned this pull request Apr 28, 2022

🐛 BUG: public folder doesn't get copied to subpath if buildOptions.site is provided in development #2409

Closed

tony-sull pushed a commit that referenced this pull request May 2, 2022

Revert "Improvements to build and dev when building for subpaths (#3156…

cf9a17a

…)" This reverts commit 637919c.

tony-sull pushed a commit that referenced this pull request May 3, 2022

Revert "Improvements to build and dev when building for subpaths (#3156…

6901196

…)" This reverts commit 637919c.

tony-sull pushed a commit that referenced this pull request May 3, 2022

Revert "Improvements to build and dev when building for subpaths (#3156…

195bbc7

…)" This reverts commit 637919c.

tony-sull pushed a commit that referenced this pull request May 4, 2022

Revert "Improvements to build and dev when building for subpaths (#3156…

6eedfcc

…)" This reverts commit 637919c.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements to build and dev when building for subpaths #3156

Improvements to build and dev when building for subpaths #3156

tony-sull commented Apr 20, 2022 •

edited

Loading

changeset-bot bot commented Apr 20, 2022 •

edited

Loading

tony-sull Apr 20, 2022

FredKSchott Apr 20, 2022

tony-sull Apr 21, 2022

natemoo-re Apr 20, 2022

tony-sull Apr 20, 2022

FredKSchott Apr 20, 2022

natemoo-re Apr 20, 2022

tony-sull Apr 20, 2022

natemoo-re Apr 20, 2022

tony-sull Apr 20, 2022

matthewp commented Apr 21, 2022

Improvements to build and dev when building for subpaths #3156

Improvements to build and dev when building for subpaths #3156

Conversation

tony-sull commented Apr 20, 2022 • edited Loading

Changes

Testing

Docs

changeset-bot bot commented Apr 20, 2022 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewp commented Apr 21, 2022

tony-sull commented Apr 20, 2022 •

edited

Loading

changeset-bot bot commented Apr 20, 2022 •

edited

Loading