-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 builds with outDir outside of cwd #4736
Conversation
🦋 Changeset detectedLatest commit: dcd9e68 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
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 |
Given that we cannot build outside of the projectRoot when |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, had one small suggestion but not blocking in any way.
import type { AstroConfig, RouteType } from '../../@types/astro'; | ||
import { appendForwardSlash } from '../../core/path.js'; | ||
|
||
const STATUS_CODE_PAGES = new Set(['/404', '/500']); | ||
const FALLBACK_OUT_DIR_NAME = './.astro-temp/'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be fine with .astro
, personally? Not a huge deal but it's a bit more clean.
const FALLBACK_OUT_DIR_NAME = './.astro-temp/'; | |
const FALLBACK_OUT_DIR_NAME = './.astro/'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this too. I thought that in the case where the build fails, and .astro-temp
is not removed (for debugging), it sends a clearer message that it's a temporary folder that can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine with me! I'd probably also add the directory to the .gitignore
files in our examples just to be safe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@natemoo-re I updated to use .astro
. I think a .gitignore
change shouldn't be needed at the meantime since this special directory is only created if someone specifies a outDir
outside of the project. Perhaps if there's a request for it you can add that too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go ahead and error when attempting this with output: 'server'
@matthewp Actually through further testing, |
Changes
Fix #4006
If
outDir
is out ofprocess.cwd()
, build the ssr output to<cwd>/.astro
instead. This directory will be removed after the build, unless an error occurs during the build, which it's preserved for debuggability.Testing
No test added as the monorepo structure makes it tricky to test this edge case (brings false positive). I did test with a separate project though and it works.
Docs
This PR handles static output only (output: 'static'
). For server output, theoutDir
MUST be inside the current working directory as most integrations requires this for the deployment output to work. (e.g. netlify, vercel, cloudflare)Should we document this? On the other hand, settingoutDir
outside of cwd feels like a special case./cc @withastro/maintainers-docs for feedback!