Skip to content
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

Merged
merged 8 commits into from
Aug 18, 2021

Conversation

ascorbic
Copy link
Contributor

@ascorbic ascorbic commented Aug 5, 2021

Description

This changes the location of generated Netlify render function to the internal functions directory (.netlify/functions-internal), rather than using the user functions directory. This means the user can use the normal functions directory for their own Netlify functions without them being deleted at build time.
Fixes #1249

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Aug 5, 2021

🦋 Changeset detected

Latest commit: 12e3766

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

This PR includes changesets to release 1 package
Name Type
@sveltejs/adapter-netlify 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


```toml
[build]
command = "npm run build"
publish = "build/publish/"
functions = "build/functions/"
Copy link
Member

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 CLI

Copy link
Member

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

Copy link
Contributor Author

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. The functions directory isn't needed anymore, because we're using the internal functions directory instead, which is hard-coded. The idea is that the functions value is for user functions, while .netlify/functions-internal is for auto-generated functions. If we remove the publish dir it will default to static because that's the value specified for SvelteKit. Should that be changed?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

@ascorbic ascorbic Aug 18, 2021

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:

  • Always write out the generated functions to .netlify/functions-internal
  • Read "publish" from netlify.toml, but if it's missing then default to "build"
  • Add more warnings and checks, such as ensuring that publish isn't set to the site root (that would delete the site!)
  • Updated the README

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 if cwd isn't process.cwd() This would likely apply to most adapters, as they all seem to assume paths are relative to process.cwd().

Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@ascorbic ascorbic force-pushed the netlify-functions-dir branch 3 times, most recently from 35688e2 to a8863d0 Compare August 18, 2021 07:55
@ascorbic
Copy link
Contributor Author

ascorbic commented Aug 18, 2021

The build is failing because the netlify.toml in the template has been updated to the new format, but the package hasn't.

@ascorbic ascorbic changed the title [feat] allow Netlify functions to coexist [feat] use Netlify internal functions directory for generated functions Aug 18, 2021
@benmccann
Copy link
Member

Thanks for this! Appreciate your knowledge of the Netlify internals and explaining it all to me

@benmccann benmccann merged commit af88239 into sveltejs:master Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is it possible to add custom netlify functions
2 participants