[Blocked] Fix handling of public resources with subpath #2348

tadeuzagallo · 2022-01-08T21:12:18Z

Changes

Change the Vite configuration to account for the subpath, this fixes the handing for resources in the public folder.
Change vite-plugin-build-html to prefix URLs in HTML attributes such as img src to match the behavior of Vite. This keeps the behavior consistent between astro dev and astro build.

Testing

I added tests to both the dev routing and preview routing. Notice that there are 5 skipped tests in the preview routing due to an existing bug in the routing, I'll try to tackle that next.

Docs

Bug fix, no changes needed to the docs

changeset-bot · 2022-01-08T21:12:32Z

🦋 Changeset detected

Latest commit: 4b5a823

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

netlify · 2022-01-09T10:45:47Z

✔️ Deploy Preview for astro-www ready!

🔨 Explore the source changes: 9ed4d72

🔍 Inspect the deploy log: https://app.netlify.com/sites/astro-www/deploys/61dc94c68c6aa000084f6ed9

😎 Browse the preview: https://deploy-preview-2348--astro-www.netlify.app

netlify · 2022-01-09T10:47:04Z

✔️ Deploy Preview for astro-docs-2 ready!

🔨 Explore the source changes: 9ed4d72

🔍 Inspect the deploy log: https://app.netlify.com/sites/astro-docs-2/deploys/61dc94c607c00f00086fc097

😎 Browse the preview: https://deploy-preview-2348--astro-docs-2.netlify.app

FredKSchott · 2022-01-10T16:52:27Z

Nice! cc @jonathantneal to take a look

jonathantneal · 2022-01-10T16:57:58Z

@FredKSchott ✅ Oh yea. Thank you, @tadeuzagallo! I’m hoping to have this one fast follow some Astro Preview fixes. :)

matthewp · 2022-01-10T17:25:25Z

Can you clarify what problem this fixes? The problem it appears to me is that you are writing <img src="/images/penguin.png" /> and you want it to be transformed into <img src="/blog/images/penguin.png" />.

I don't think we should be rewriting every link like this because it will not work in SSR. We are actually moving away from rewriting HTML at all.

The way to get your subpath prefixed is to use Astro.site like so:

<img src={new URL('./images/penguin.png', Astro.site).pathname} />

Or just include /blog in your URL:

<img src="/blog/images/penguin.png" />

tadeuzagallo · 2022-01-10T17:31:19Z

Sorry, I explained in more depth in the discord channel. Here's the current issue with image loading:

Let's assume we have the subpath set to /blog/ and that there's there's an image in public/penguin.png. In the current state of things you can't load /blog/penguin.png, only /penguin.png.

The way I tried to fix this was by passing the subpath configuration down to Vite through the base configuration option: https://vitejs.dev/config/#base. The automatic prefixing behavior that I implemented was to mimic Vite's behavior as described here: https://vitejs.dev/guide/build.html#public-base-path

FredKSchott · 2022-01-10T20:26:57Z

packages/astro/src/core/dev/index.ts

+			return;
+		}
+		pathname = pathname.substr(this.devRoot.length) || '';
+		req.url = pathname.startsWith('/') ? pathname : '/' + pathname;


if we go this route, make sure that no one is using originalUrl internally that needs this new req.url instead https://expressjs.com/en/api.html#req.originalUrl

I'll do some grepping.

tadeuzagallo · 2022-01-10T20:30:39Z

I removed the unnecessary regexp tests and changes to the .astro pages of the test fixtures.

jonathantneal · 2022-01-10T21:25:12Z

Does this patch redirect subpath requests to the root? So that /blog/twitter.png is treated like /twitter.png?

That is a very clever patch. It will be more fragile, as it relies on incorrect behavior throughout the rest of the stack.

If this occurs before other plugins, including user plugins, then I could imagine other unintended side-effects, from plugins that intercept URLs like /__VITE__SOMETHING/asset.png.

jonathantneal · 2022-01-10T21:30:45Z

packages/astro/src/core/dev/index.ts

+
+		let pathname = req.url || '/';
+		if (!pathname.startsWith(this.devRoot)) {
+			this.renderError(req, res, next);


Could this next() instead? I understand (multiple) plugins create unique URLs like /__VITE_FILE__/something, and my concern is that this would error them out before they had a chance to intercept the URL.

Ref: https://vitejs.dev/guide/api-plugin.html#conventions

My concern with that is that /image.png would still work in dev, but break in production, which could lead to bugs.

Yes. We don’t want either scenario, I think. We can’t predict what other plugins will do, but plugins following the documented conventions would probably get caught by this plugin. 😅

IMHO, this needs to get fixed higher up, wherever the problem originates.

Yeah we shouldn't ban all non-root pathnames, as Vite is going to send some /@fs and /.vite paths. I wouldn't want to special-case them either.

It would be ideal to ban /image.png but shouldn't we get this fix in first (using next() here), as it improves subpath support, and then do the ideal fix afterwards? The ideal fix doesn't seem obvious to me.

If that is the case, then the ideal fix may be the necessary fix.

Backing up here: have we confirmed that just setting base in the dev createVite() call doesn't fix this issue, without all of the other changes in this PR? I just tried that and it seems to accomplish our goals here: on the docs site /main.css becomes a 404 and /docs/main.css becomes a 200.

If you set the base in vite, when using the astro dev, it will transform all the img src's and the link as part of transformIndexHtml to automatically prefix them, which I believe we agreed is an undesirable behavior (and at the very least doesn't match astro build). I created a very simple test repo which I believe illustrates all the issues with just a couple css files and an image: http://github.com/tadeuzagallo/blog

It could be worth playing around with transformIndexHTML then. It's function signature may have been designed to solve for this: https://github.com/vitejs/vite/blob/647168b2b44b82b1a1cbd8e639f74ddf52a5d5cd/packages/vite/src/node/server/index.ts#L198

transformIndexHtml( url: string, html: string, originalUrl?: string ): Promise<string>

Somewhere else in the codebase, we run it like this:

// run transformIndexHtml() in dev to run Vite dev transformations if (mode === 'development' && !astroConfig.buildOptions.experimentalStaticBuild) { const relativeURL = filePath.href.replace(astroConfig.projectRoot.href, '/'); html = await viteServer.transformIndexHtml(relativeURL, html, pathname); }

so maybe the dev call to this function just needs to change the first or third argument to handle this?

I could be wrong, I'll dig some more, but from what I read of the code it's purely based on viteConfig.base. Changing the base around the call in the hackiest way did "fix" the behavior:

const base = viteServer.config.base; viteServer.config.base = '/'; html = await viteServer.transformIndexHtml(relativeURL, html, pathname); viteServer.config.base = base;

wbern · 2022-01-17T19:36:31Z

I'm trying to deploy to a site that requires me to deploy to a sub path, I assume this PR fixes that? I tried doing manual copy commands but the dynamic vendor script still tries to load things without the appended sub path.

tadeuzagallo · 2022-01-17T20:56:52Z

@wbern when you say deploy, are you talking about astro build? This only affects dev.

wbern · 2022-01-17T21:10:05Z

@wbern when you say deploy, are you talking about astro build? This only affects dev.

Yes, build. Ok, because the siteOptions don't seem to help my case for the build.

tadeuzagallo · 2022-01-17T21:12:07Z

@wbern in all my tests astro build worked fine with subpaths. If you have a repro or some more info it might be a good idea to file an issue.

wbern · 2022-01-20T21:17:43Z

@wbern in all my tests astro build worked fine with subpaths. If you have a repro or some more info it might be a good idea to file an issue.

I have opened it here, thank you for the advice. #2434

jonathantneal · 2022-01-21T00:15:34Z

This PR has been held up because we know we shouldn’t (and probably can’t) block requests outside of the subpath. However, it looks like there’s a problem in the current solution if it doesn’t and those requests get passed along.

jonathantneal · 2022-01-21T00:23:09Z

@tadeuzagallo, I think you could either refactor this with an approach that does not block requests, or you could do that in a separate PR, if this one has lingered too long and some of the blocking information is getting lost.

jonathantneal · 2022-02-07T18:41:36Z

This may have been handled in #2494, and we’ll need to revisit this if it comes back up.

This was a tough one. Thank you for all of your hard effort, @tadeuzagallo.

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

tadeuzagallo force-pushed the subpath-assets branch from 1e50874 to 4b5a823 Compare January 9, 2022 10:43

jonathantneal self-assigned this Jan 10, 2022

tadeuzagallo force-pushed the subpath-assets branch from 4b5a823 to 9ed4d72 Compare January 10, 2022 20:19

FredKSchott reviewed Jan 10, 2022

View reviewed changes

Fix handling of public resources with subpath

2faf58f

tadeuzagallo force-pushed the subpath-assets branch from 9ed4d72 to 2faf58f Compare January 10, 2022 20:29

jonathantneal reviewed Jan 10, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

tadeuzagallo mentioned this pull request Jan 17, 2022

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

Closed

jonathantneal changed the title ~~Fix handling of public resources with subpath~~ [Blocked] Fix handling of public resources with subpath Jan 25, 2022

FredKSchott mentioned this pull request Jan 29, 2022

refactor dev to use vite server #2494

Merged

jonathantneal closed this Feb 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Blocked] Fix handling of public resources with subpath #2348

[Blocked] Fix handling of public resources with subpath #2348

tadeuzagallo commented Jan 8, 2022

changeset-bot bot commented Jan 8, 2022 •

edited

Loading

netlify bot commented Jan 9, 2022 •

edited

Loading

netlify bot commented Jan 9, 2022 •

edited

Loading

FredKSchott commented Jan 10, 2022

jonathantneal commented Jan 10, 2022

matthewp commented Jan 10, 2022 •

edited

Loading

tadeuzagallo commented Jan 10, 2022 •

edited

Loading

FredKSchott Jan 10, 2022 •

edited

Loading

tadeuzagallo Jan 10, 2022

tadeuzagallo commented Jan 10, 2022

jonathantneal commented Jan 10, 2022 •

edited

Loading

jonathantneal Jan 10, 2022 •

edited

Loading

tadeuzagallo Jan 10, 2022

jonathantneal Jan 10, 2022

matthewp Jan 11, 2022

matthewp Jan 11, 2022 •

edited

Loading

jonathantneal Jan 11, 2022

FredKSchott Jan 11, 2022

tadeuzagallo Jan 11, 2022

FredKSchott Jan 11, 2022

tadeuzagallo Jan 11, 2022

This comment has been minimized.

wbern commented Jan 17, 2022

tadeuzagallo commented Jan 17, 2022

wbern commented Jan 17, 2022

tadeuzagallo commented Jan 17, 2022

wbern commented Jan 20, 2022

jonathantneal commented Jan 21, 2022

jonathantneal commented Jan 21, 2022

jonathantneal commented Feb 7, 2022 •

edited

Loading

[Blocked] Fix handling of public resources with subpath #2348

[Blocked] Fix handling of public resources with subpath #2348

Conversation

tadeuzagallo commented Jan 8, 2022

Changes

Testing

Docs

changeset-bot bot commented Jan 8, 2022 • edited Loading

🦋 Changeset detected

netlify bot commented Jan 9, 2022 • edited Loading

netlify bot commented Jan 9, 2022 • edited Loading

FredKSchott commented Jan 10, 2022

jonathantneal commented Jan 10, 2022

matthewp commented Jan 10, 2022 • edited Loading

tadeuzagallo commented Jan 10, 2022 • edited Loading

FredKSchott Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tadeuzagallo commented Jan 10, 2022

jonathantneal commented Jan 10, 2022 • edited Loading

jonathantneal Jan 10, 2022 • edited Loading

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 Jan 11, 2022 • edited Loading

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

This comment has been minimized.

wbern commented Jan 17, 2022

tadeuzagallo commented Jan 17, 2022

wbern commented Jan 17, 2022

tadeuzagallo commented Jan 17, 2022

wbern commented Jan 20, 2022

jonathantneal commented Jan 21, 2022

jonathantneal commented Jan 21, 2022

jonathantneal commented Feb 7, 2022 • edited Loading

changeset-bot bot commented Jan 8, 2022 •

edited

Loading

netlify bot commented Jan 9, 2022 •

edited

Loading

netlify bot commented Jan 9, 2022 •

edited

Loading

matthewp commented Jan 10, 2022 •

edited

Loading

tadeuzagallo commented Jan 10, 2022 •

edited

Loading

FredKSchott Jan 10, 2022 •

edited

Loading

jonathantneal commented Jan 10, 2022 •

edited

Loading

jonathantneal Jan 10, 2022 •

edited

Loading

matthewp Jan 11, 2022 •

edited

Loading

jonathantneal commented Feb 7, 2022 •

edited

Loading