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.
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
[feat] use Netlify internal functions directory for generated functions #2113
[feat] use Netlify internal functions directory for generated functions #2113
Changes from 3 commits
e228bbc
7ecb000
2bcbeb7
b730c54
bb5939d
b10fcb9
4fe2486
12e3766
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.
I don't know if we can just remove this. Where will it put functions then? We put this here on purpose so that all output would show up under
build
and be automatically gitignored without the user having to edit their.gitignore
at all. It would be nice if that continued to work for people who are not using the Netlify CLIThere 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.
Or at least if we're going to remove this line, should we also remove the
publish
line so that ends up under.netlify/publish
or something? It seems weird to treat these inconsistently.Perhaps what we could do is maintain the old behavior and use the directories in these files if they are present and if they are not then we use the directory you're specifying as a default
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.
The functions will be in
.netlify/functions-internal
. Thefunctions
directory isn't needed anymore, because we're using the internal functions directory instead, which is hard-coded. The idea is that thefunctions
value is for user functions, while.netlify/functions-internal
is for auto-generated functions. If we remove thepublish
dir it will default tostatic
because that's the value specified for SvelteKit. Should that be changed?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 they user isn't using the Netlify CLI then presumably this won't be an issue anyway, as it wouldn't be using the Netlify preset when building locally as the auto-detection won't trigger it.
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.
Though I suppose the user could have manually specified the preset.
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.
Ok, thanks for clarifying and being patient as I come up-to-speed on Netlify stuff. I'm on board with your plan. Lets always output to
.netlify/functions-internal
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.
OK, so skip the CLI detection entirely?
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.
What would be the benefit of CLI detection? If we're not using the Netlify CLI, I'm assuming that any code
.netlify/functions-internal
will still be used?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.
Correct. I'll update the 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.
OK, I've updated the PR as follows:
.netlify/functions-internal
"publish"
fromnetlify.toml
, but if it's missing then default to"build"
One thing I'd like to suggest for a follow-up PR is that
cwd
is passed to adapter functions here. That would allow us to ensure that we resolve all the paths correctly inside adapters. I may be misunderstanding it as I've not tested it, but as it stands I'm not sure if these will all work ifcwd
isn'tprocess.cwd()
This would likely apply to most adapters, as they all seem to assume paths are relative toprocess.cwd()
.