-
-
Notifications
You must be signed in to change notification settings - Fork 35
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: disable no-undef
ESLint rule in TypeScript project
#483
Conversation
🦋 Changeset detectedLatest commit: 4fbf70f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm missing something, but in #481 you are writing about no-undef
, and your PR seems to mix and match no-undef
and no-unused-vars
randomly.
More importantly, I deleted the eslint.config.js
in the repro provided and run pnpm dlx https://pkg.pr.new/sveltejs/cli/sv@483
which still revealed the original error.
I'm assuming your PR was meant for no-undef
?
no-unused-vars
ESLint rule in TypeScript projectno-undef
ESLint rule in TypeScript project
@manuel3108 Sorry, I got confused for some reason. Fixed now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the one thing, everything seems to be working as expected now.
Edit: I can have a look later this week if you want
packages/addons/eslint/index.ts
Outdated
@@ -85,11 +85,25 @@ export default defineAddon({ | |||
const globalsNode = common.createSpreadElement(common.expressionFromString('globals.node')); | |||
const globalsObjLiteral = object.createEmpty(); | |||
globalsObjLiteral.properties = [globalsBrowser, globalsNode]; | |||
const off = common.createLiteral('off'); | |||
off.comments = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also thought about whether there was a better way, but it seemed a bit difficult. Since it gets formatted correctly by Prettier, as you mentioned, I decided to leave it as it is.😅
I will take a closer look at it tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem is that not everyone has prettier installed, and we only run prettier if prettier was already setup by the user or the cli. So there is a chance users will see this.
Thanks for your help!
909905e
to
827337c
Compare
I think the format is much better now. The only remaining issue is the indentation, but I this we can split the PR to fix it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats perfect now, thanks!
Indentation should be fixed by #380 in theory, so I don't think we need to bother about this now, as you said!
close: #481