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

chore: rm function dir in template ignore files #1436

Closed

Conversation

jthegedus
Copy link
Contributor

@jthegedus jthegedus commented May 12, 2021

Remove /function from the ignore file.

This seems to no longer be used for anything and currently conflicts with the Firebase Adapter standard default usage (Cloud Functions are defined by a dir called functions but is configurable) so requires a documentation/manual step to rectify.


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

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

@benmccann
Copy link
Member

Hmm, I think other adapters such as adapter-netlify create a functions directory that is not meant to be checked in, so I'm not quite sure what to do about these conflicting requirements

It's cool to see this adapter. You should list it on https://github.com/sveltejs/integrations or https://sveltesociety.dev/ 😄

@jthegedus
Copy link
Contributor Author

jthegedus commented May 13, 2021

@benmccann My argument/proposition would be that the template apps should only ignore files in the template and not pre-empt output distinct to some adapters.

so I'm not quite sure what to do about these conflicting requirements

I am not asking for my specific requirement to be supported here, just that no specific requirements of adapters supported in the SvelteKit template.

Processes like Svelte Adders are perfect to address this for all adapters adding to ignore files as necessary.


It's cool to see this adapter. You should list it on sveltejs/integrations or sveltesociety.dev

I will add links to the adapter once SvelteKit is 1.0 as I'm already overwhelmed with user reported issues and keeping the adapter updated to latest SvelteKit changes.

@benmccann
Copy link
Member

I hear what you're saying and it's a pretty reasonable to suggest that the template not have any adapter-specific code in it. At the same time we want to make setup really easy and don't want to have to create svelte adders just to use an adapter. And there should be some consistency between the different adapters as well. It would be preferable not to have the functions directory generated in some cases and written by hand in other cases. It's also a design goal that you should mostly be able to write a SvelteKit app, swap the adapter, and have it work on another hosting platform. I think the issue here is that the Firebase adapter doesn't have support for SvelteKit endpoints yet (https://github.com/jthegedus/svelte-adapter-firebase#faq). My preference would be that we try to fix that and make the functions directory generated from endpoints rather than tweaking the default .gitignore

@jthegedus
Copy link
Contributor Author

jthegedus commented May 17, 2021

It would be preferable not to have the functions directory generated in some cases and written by hand in other cases

I would agree, except that Netlify (by default) specifies functions/ as an output dir for their compiled API code, whereas Firebase (by default) uses functions/ as the source and destination of the compiled code, usually functions/src and/or functions/lib, so 99.999% of the time is checked in and has it's own ignore file inside generated by Firebase. By virtue of the fact one first-party-maintained adapter uses functions/ shouldn't mean every other adapter which doesn't conform should have to instruct users to delete ignore statements from the template.

I think the issue here is that the Firebase adapter doesn't have support for SvelteKit endpoints yet (https://github.com/jthegedus/svelte-adapter-firebase#faq)

This is actually untrue, endpoints are supported. The confusion here comes from the initial comparison of SvelteKit endpoints to Next.js API routes. Next.js routes on Vercel are each individual AWS Lambdas. The initial comparison lead me to believe the .svelte-kit/output/server/ output would be individual .js files for each endpoint where I would be able to output a GCP Cloud Function for each endpoint. This is not the case with the current pattern. The caveat you link in the Firebase adapter was for a few reasons:

  1. I am not actively solving problems related to the Firebase JS SDK v8, people should use v9 as noted on the SvelteKit FAQ/
  2. SvelteKit endpoints are not the same as Next.js routes and are not separate Cloud Functions, the whole SvelteKit server app is run in a single Cloud Function.

I need to update the FAQ, it was written quite a while ago in terms of SvelteKit releases.

With that explanation, hopefully you see this is nothing to do with the adapter, but in fact, the target platforms. Netlify & Firebase (by default) use the same dir for similar but incompatible reasons.

My argument is again, that a single first-party-maintained adapter, should not force the SvelteKit template .gitignore items.

In addition to this, the Netlify functions dir is configurable, so the ommission with .gitignore is only useful to a subset of that adapter's users.


I am just trying to avoid writting code or instructions to work around this issue. atm I have written code or have extra steps in my instructions to work around 3 separate issues.

@jthegedus
Copy link
Contributor Author

Perhaps part of the adapter API should be a utility function to check & write ignore items to the SvelteKit root .gitignore file. That way each adapter can read their respective platform configs and insert their respective build output dirs as ignore items?

@benmccann
Copy link
Member

Yeah, that sounds like a reasonable solution. I'd be in favor of this change if we can make the other adapters check and modify the .gitignore

@benmccann benmccann added the adapters - general Support for functionality general to all adapters label May 19, 2021
@jthegedus
Copy link
Contributor Author

This change has since been implemented in #1499 whether by accident or not, I agree with the change 😉

@jthegedus jthegedus closed this May 26, 2021
@jthegedus jthegedus deleted the remove-function-dir-ignore branch August 2, 2021 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adapters - general Support for functionality general to all adapters
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants