This repository has been archived by the owner on Feb 10, 2025. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
feat(netlify): support included/excluded files #325
feat(netlify): support included/excluded files #325
Changes from 8 commits
e7206e2
cedb642
bc08199
171b832
4db5b4d
4cdf7ae
4595d84
2c98705
76fab50
d68f5b2
5570bdf
1d198eb
1a2319f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Are we sure it can't break any existing app?
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.
As far as I can work out, existing apps that rely on cwd would either have unchanged behaviour (if they're not a monorepo), or would already be broken (if they are using a monorepo)
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 don't understand what the "correct paths at runtime" refers to here. It feels dangerous to call
chdir()
as a request handler. Can you explain where is resolving from the cwd?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.
If a site uses filesystem functions at runtime, this is the only way it can correctly load the files
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 don't think using filesystem functions is something Astro supports in the runtime? Files could be moved during bundling and it wouldn't work later.
Is
process.chdir()
needed for this PR?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.
We don't prevent people using fs functions, so I'd imagine lots of people do. This PR was prompted by an issue where a library was trying to load i18n data and it wasn't found. This part fixes precisely that issue: of files being moved during bundling. Without that there's not a lot of point to allowing users to include files in the bundle, because the user code won't be able to find them.
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 don't feel like fs functions are a common thing especially when dealing in a bundled runtime. There's Vite APIs like
import.meta.glob
,?url
, and?raw
for them. Fs functions are also likely to fail elsewhere than only on Netlify, and so we shouldn't support and encourage them.I figured the benefit with
includeFiles
andexcludeFiles
are like Vercel's option, because nft has trouble sometimes detecting every file. Especially inside node_modules where libraries use fs relatively, those code are not processed by Vite so we could use the option for them, however if they resolve relatively I'm not sure how cwd helps here. And if the library do resolve from cwd, it would've been generally a problem if an Astro is also hosted on a subdirectory or any other fine setup.I'm not really comfortable with keeping this in the adapter. It feels surprising to me that it could change the cwd internally and on every request.
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.
They are like Vercel's option. In fact they use almost the same implementation. With libraries that use fs functions, they usually accept a directory as an option or resolve relative to cwd. It's very common for i18n libraries such as i18next, for example.
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.
If we're about to do a major I can do a breaking changeset for the chdir part.