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

Add linting & formatting #7

Closed
babichjacob opened this issue Feb 13, 2021 · 5 comments
Closed

Add linting & formatting #7

babichjacob opened this issue Feb 13, 2021 · 5 comments
Labels

Comments

@babichjacob
Copy link
Member

In my opinion, this should be in charge of implementing #6 or #5 depending on which works best.

That is to say, there should ideally only be svelte-add/linting and not svelte-add/eslint nor svelte-add/prettier

@babichjacob babichjacob added the straightforward Good for newcomers label Mar 13, 2021
@manuel3108
Copy link
Member

I have started some work here: https://github.com/manuel3108/svelte-add-linting

I think it's pretty hard, because the svelte template and prettier are both opinionated.
That's why I choose to try to approach the svelte template as close as possible, by i.e. using tabs instead of spaces.

I'm quite happy with the results of prettier now, as it only modifies 4 now, instead of nearly every file when i was starting.

Eslint is currently giving me this error:

/mnt/files/dev/web/sveltekit/src/routes/index.svelte
  2:22  error  Unable to resolve path to module '$components/Counter.svelte'  import/no-unresolved

If you have an idea, please let me know.

Also I'm unsure for the scripts added. For the my past projects I added them like this:

		"lint": "eslint .",
		"format": "prettier --write .",
		"format-check": "prettier --check .",
		"check": "npm run lint && npm run format-check"

Any opinion on this?

@babichjacob
Copy link
Member Author

I have started some work here: https://github.com/manuel3108/svelte-add-linting

I think it's pretty hard, because the svelte template and prettier are both opinionated.
That's why I choose to try to approach the svelte template as close as possible, by i.e. using tabs instead of spaces.

I'm quite happy with the results of prettier now, as it only modifies 4 now, instead of nearly every file when i was starting.

Eslint is currently giving me this error:

/mnt/files/dev/web/sveltekit/src/routes/index.svelte
  2:22  error  Unable to resolve path to module '$components/Counter.svelte'  import/no-unresolved

If you have an idea, please let me know.

I do not. Problems like this are why I was personally afraid of being responsible for the linting adder. Leaving it to solve later is fine by me.

Also I'm unsure for the scripts added. For the my past projects I added them like this:

		"lint": "eslint .",
		"format": "prettier --write .",
		"format-check": "prettier --check .",
		"check": "npm run lint && npm run format-check"

Any opinion on this?

I agree with combining linting and format checking into one script ("check") because they really are 2 just parts of the same step. I'd leave the name check open to type checking, though, so I'd personally rather have scripts like this:

"format": "prettier --write .",
"format:check": "prettier --check .",
"eslint": "eslint .",
"lint": "run-p format:check eslint",

and only tell people to use lint and format (the other two are just "implementation details").

What are your thoughts on that? Do you also know if it's always going to be the case that Prettier formats code in a way that ESLint accepts, or can people get caught on errors from that? I personally have only used ESLint's formatter to prevent such a thing from ever occurring.

@mattlehrer
Copy link

mattlehrer commented Mar 15, 2021

I'm excited about this one. I played around with the no-unresolved issue and I don't see a way to get eslint or eslint-plugin-import to resolve those imports. It's expecting to use a resolver package, and the config is just a Vite alias.
https://github.com/benmosher/eslint-plugin-import/blob/master/README.md#resolvers

You can ignore those imports like this, in .eslintrc.json:

"rules": {
	"import/no-unresolved": ["off", { "ignore": ["$\\w/"] }]
}

I set it up that way instead of $components because SvelteKit changed the components path to lib now. It could be more specific if that feels off:
"import/no-unresolved": ["off", { "ignore": ["${lib|components}/"] }]

@manuel3108
Copy link
Member

I played around with the no-unresolved issue and I don't see a way to get eslint or eslint-plugin-import to resolve those imports. It's expecting to use a resolver package, and the config is just a Vite alias.

Ahhh, you where there a bit too early 😄

I found another way to remove this error: https://github.com/manuel3108/svelte-add-linting/commit/61e1a6ea200a54b65ff9e60f6e27e734d18f3941

Essentially I am using eslint-import-resolver-alias and adding a bit of config to the eslintrc.json.

The tricky thing is the $ in the $lib folders name. The package I am using, does not escape the string properly, that's why we need to escape it "map": [["\\$lib", "./src/lib"]].
Ideally we would create a PR for this upstream package, but the maintainer seems very inactive, so I'm not planning to do so.

I agree with combining linting and format checking into one script ("check") because they really are 2 just parts of the same step. I'd leave the name check open to type checking, though, so I'd personally rather have scripts like this:

"format": "prettier --write .",
"format:check": "prettier --check .",
"eslint": "eslint .",
"lint": "run-p format:check eslint",
and only tell people to use lint and format (the other two are just "implementation details").

I agree mainly, but I don't think we should use run-p here. It would add an additional dependency, and it might happen that the output of eslint and prettier is mixed together. I have changed the lint script to run them one after the other

and only tell people to use lint and format (the other two are just "implementation details").

Updated the readme

Do you also know if it's always going to be the case that Prettier formats code in a way that ESLint accepts, or can people get caught on errors from that?

That's why we are using eslint-config-prettier as this disables all eslint rules that would cause conflicts.

@babichjacob
Copy link
Member Author

The default create-svelte process includes linting and formatting options now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants