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(addons): Respect .gitignore in eslint addon #335

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

ieedan
Copy link

@ieedan ieedan commented Nov 27, 2024

Fixes #334

Searched the docs and tried the adapters out ended up also adding .wrangler and .netlify

Copy link

changeset-bot bot commented Nov 27, 2024

🦋 Changeset detected

Latest commit: cdf49b6

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

This PR includes changesets to release 1 package
Name Type
sv 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

@ieedan ieedan changed the title feat: Add .vercel to ignores in eslint addon feat: Add @sveltejs/<adapter> out directories to eslint ignores. Nov 27, 2024
@AdrianGonz97
Copy link
Member

thank you! however I'm not sure if having to manually update the ignores property for every new entry is the best solution (also, TIL that eslint v9 no longer takes the .gitignore file into consideration, which is fun).

perhaps it would be nicer to have the .gitignore automatically included using the @eslint/compat package?

It can be implemented as so:

import { fileURLToPath } from "node:url";
import { includeIgnoreFile } from "@eslint/compat";

const gitignorePath = fileURLToPath(new URL("./.gitignore", import.meta.url));
export default {
 // ...
 includeIgnoreFile(gitignorePath)
}

thoughts? @manuel3108 @benmccann

@manuel3108
Copy link
Member

Yeah, i think that approach would make sense, although we will have basically the same problem in the .gitignore file then. But personally, I don't think this should be implemented in a "hardcoded" way. I would prefer to see this implemented together with #248

@AdrianGonz97
Copy link
Member

AdrianGonz97 commented Nov 27, 2024

I don't think this should be implemented in a "hardcoded" way. I would prefer to see this implemented together with #248

the .gitignore already includes all of the different adapter output dirs anyway (or at least it should, in the case of netlify which needs to be fixed)

@manuel3108
Copy link
Member

the .gitignore already includes all of the different adapter output dirs anyway (or at least it should, in the case of netlify which needs to be fixed)

At this point for me it doesn't look like it's including the vercel output. So that's also missing. But I wouldn't be in favor of adding that globally to the templates, as explained above.

@AdrianGonz97
Copy link
Member

At this point for me it doesn't look like it's including the vercel output. So that's also missing.

here's the currently generated .gitignore:

node_modules

# Output
.output
.vercel
/.svelte-kit
/build

# OS
.DS_Store
Thumbs.db

# Env
.env
.env.*
!.env.example
!.env.test

# Vite
vite.config.js.timestamp-*
vite.config.ts.timestamp-*

the only one that's missing is .netlify (and maybe wrangler? i normally use @sveltejs/adapter-cloudflare which outputs into .svelte-kit/cloudflare instead, so idk).

But I wouldn't be in favor of adding that globally to the templates, as explained above.

yea but that would only be helpful for people who are changing adapters via add and not manually. why not just get them all in one go instead? we're already most of the way there anyway since we're only missing 1 or 2 entries.

@manuel3108
Copy link
Member

here's the currently generated .gitignore:

I'm blind, as always!

yea but that would only be helpful for people who are changing adapters via add and not manually. why not just get them all in one go instead? we're already most of the way there anyway since we're only missing 1 or 2 entries.

Good point about the manual stuff, this should be the way.

@ieedan
Copy link
Author

ieedan commented Nov 27, 2024

I just went through and ran all of the adapters to see what they output since I don't think any of those outputs are actually in the docs for the adapters at least that I could find.

@AdrianGonz97
Copy link
Member

AdrianGonz97 commented Nov 27, 2024

here are the generated dirs in the root:

@sveltejs/adapter-node - /build
@sveltejs/adapter-static - /build
@sveltejs/adapter-cloudflare - /.wrangler 
@sveltejs/adapter-cloudflare-workers /.wrangler
@sveltejs/adapter-netlify - /.netlify
@sveltejs/adapter-vercel - /.vercel

not sure what the .output dir that already exists in the .gitignore is supposed to be for, but we only need to add .wrangler and .netlify to the gitignore.

@ieedan
Copy link
Author

ieedan commented Nov 27, 2024

Is there a reason there can't just be a exported constant somewhere that has all of these ignores to be used everywhere?

@AdrianGonz97
Copy link
Member

Is there a reason there can't just be a exported constant somewhere that has all of these ignores to be used everywhere?

not exactly sure I follow - who should be exporting these? the individual adapter packages?

@ieedan
Copy link
Author

ieedan commented Nov 27, 2024

Is there a reason there can't just be a exported constant somewhere that has all of these ignores to be used everywhere?

not exactly sure I follow - who should be exporting these? the individual adapter packages?

I don't think it being a separate package would be practical but it may be nice to have an internal package to use for it? Like a @sveltejs/constants or something idk.

Otherwise you'd have to depend on 10 packages just to figure out what to ignore lol

@AdrianGonz97
Copy link
Member

eh, that would be pretty niche and not particularly helpful when it comes to the gitignore file since there's no way to 'import' those constants into it

anyway, I think we're in agreement that implementing #335 (comment) would be good, so feel free to add it to this PR!

@ieedan
Copy link
Author

ieedan commented Nov 27, 2024

I meant more it could just be passed around to build whatever files need it.

Either way let me wrap a few things up and I can give it a shot!

@ieedan ieedan changed the title feat: Add @sveltejs/<adapter> out directories to eslint ignores. feat(addons): Respect .gitignore in eslint addon Nov 27, 2024
@ieedan
Copy link
Author

ieedan commented Nov 27, 2024

This should be it but I cannot seem to get it to build or run tests on my machine I am probably just doing something wrong.

Copy link

pkg-pr-new bot commented Nov 27, 2024

Open in Stackblitz

npm i https://pkg.pr.new/sveltejs/cli/sv@335
npm i https://pkg.pr.new/sveltejs/cli/svelte-migrate@335

commit: cdf49b6

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.

feat: Add .vercel to eslint addon ignores
4 participants