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

[fix] revert adapters automatically updating .gitignore (#1924) #2055

Merged
merged 1 commit into from
Aug 1, 2021

Conversation

Rich-Harris
Copy link
Member

Apologies for missing this when #1924 was still open. It violates something I've learned the hard way to be a sacrosanct rule: a given file in a project should be edited by humans, or by machines, but never by both after the project has been created (package.json might seem like an obvious counter-example, but appending dependencies is something the package manager does as a direct consequence of your command; it doesn't get unexpected edits anywhere else). We really can't go around editing people's code, even in trivial-seeming ways in ignore files.

Adding ignored directories seems simple and uncontroversial enough to be a no-brainer. But even in #1924 itself it introduced unwanted changes:

/.svelte-kit
/build
/functions

+build

Here, the /build line already excluded the build directory from version control. The inserted line is duplicative. Worse than that, it will cover any folder called build anywhere in your project, so if your app had a route whose name coincided with that fairly common English word, it would now be excluded as well. As someone who has lost work because of incorrectly-excluded folders in the past, I would be pretty irate if it happened again because my framework took it upon itself to 'fix' my ignore file. We could robustify it, but there'll always be some edge case.

Beyond that, someone might actually want the build artifacts to be checked in. In fact I work on a team where that's the norm! (It's a practice I've railed against, but there are reasons that it's done that way, and the fact that it's a thing that people do, however unconventional, outweighs any arguments about why they shouldn't.) While the documentation tells you that you can comment the line out, most people would simply delete the line, only for it to be reinserted. Fighting your tools is a bad feeling.

Here's what I think we should do instead:

  • Where it makes sense, adapters should place their output inside .svelte-kit (i.e. .svelte-kit/netlify, etc) so that they are already ignored by ignore files
  • Check ignore files to see if we're writing to locations that aren't already excluded, and print a loud warning if they're not. We're using gitignore-parser in create-svelte to do this robustly:
    // ignore contents of .gitignore or .ignore
    if (!gitignore.accepts(name) || !ignore.accepts(name) || name === '.ignore') return;

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 1, 2021

🦋 Changeset detected

Latest commit: 23b296f

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

This PR includes changesets to release 6 packages
Name Type
@sveltejs/adapter-cloudflare-workers Patch
@sveltejs/adapter-netlify Patch
@sveltejs/adapter-node Patch
@sveltejs/adapter-static Patch
@sveltejs/adapter-vercel Patch
@sveltejs/kit 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

Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure you should revert the changeset from the original PR and this one will need it's own changeset. Other than that, LGTM

Where it makes sense, adapters should place their output inside .svelte-kit (i.e. .svelte-kit/netlify, etc) so that they are already ignored by ignore files

So obvious in retrospect. I'm embarrassed none of us thought of it in. I started us off by updating the Netlify adapter here: #2058

@benmccann
Copy link
Member

FYI @JeanJPNM, @jthegedus

@pngwn
Copy link
Member

pngwn commented Aug 1, 2021

Like @benmccann we should fix forward (even though this is a revert) and add a new changeset for this.

@JeanJPNM
Copy link
Contributor

JeanJPNM commented Aug 1, 2021

Seems reasonable, but I see this more as a bug, since the code duplicates paths that are relative to the root. But implementing that could make the process a lot slower. And manually stating that the path is relative to the root of the project on the adapter configuration feels like a waste of time. Maybe reverting the changes its the best option.

@benmccann benmccann changed the title revert #1924 [fix] revert adapters automatically updating .gitignore (#1924) Aug 1, 2021
@benmccann
Copy link
Member

Like @benmccann we should fix forward (even though this is a revert) and add a new changeset for this.

While forward fixing is nice, I think it's more important to get the change out before folks have their .gitignore files updated. We can go back and implement the new way afterwards

I pushed some changes and may merge this pretty soon to get it out

@benmccann benmccann merged commit d81de60 into master Aug 1, 2021
@benmccann benmccann deleted the revert-1924 branch August 1, 2021 21:58
@jthegedus
Copy link
Contributor

Should we expect to see gitignore-parser provided via the Adapter API in the near future?

@Rich-Harris
Copy link
Member Author

Yes, I think we should — we could have an API like utils.warn_if_unignored([opts.output])

@jthegedus
Copy link
Contributor

Sounds like a good soln. Hopefully loud CLI output is enough.

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.

5 participants