-
-
Notifications
You must be signed in to change notification settings - Fork 58
fix(valid-compile): use compiler options if provided #1373
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
Open
baseballyama
wants to merge
1
commit into
main
Choose a base branch
from
chore/svelte-config
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+37
−0
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'eslint-plugin-svelte': patch | ||
--- | ||
|
||
fix(valid-compile): use compiler options if provided |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
11 changes: 11 additions & 0 deletions
11
...int-plugin-svelte/tests/fixtures/rules/valid-compile/invalid/compiler-options-config.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
{ | ||
"languageOptions": { | ||
"parserOptions": { | ||
"svelteConfig": { | ||
"compilerOptions": { | ||
"runes": true | ||
} | ||
} | ||
} | ||
} | ||
} |
8 changes: 8 additions & 0 deletions
8
...int-plugin-svelte/tests/fixtures/rules/valid-compile/invalid/compiler-options-errors.yaml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
- message: >- | ||
`count` is updated, but is not declared with `$state(...)`. Changing its | ||
value will not correctly trigger updates | ||
|
||
https://svelte.dev/e/non_reactive_update(non_reactive_update) | ||
line: 2 | ||
column: 6 | ||
suggestions: null |
9 changes: 9 additions & 0 deletions
9
...nt-plugin-svelte/tests/fixtures/rules/valid-compile/invalid/compiler-options-input.svelte
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
<script lang="ts"> | ||
let count = 0; | ||
function increment() { | ||
count += 1; | ||
} | ||
</script> | ||
|
||
{count}<br /> | ||
<button onclick={increment} type="button">Increment</button> |
3 changes: 3 additions & 0 deletions
3
...ugin-svelte/tests/fixtures/rules/valid-compile/invalid/compiler-options-requirements.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{ | ||
"svelte": ">=5.0.0" | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
To avoid unintended behavior, can you please pass only the necessary options?
Since
generate: false
is different from the compiler used by the user, I think it is highly likely that the user's definedcompilerOptions
cannot be used as is.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.
Could you explain this in a bit more detail?
If
generate: false
is specified, the Svelte compiler just skips the transform phase, so I don’t think it would be a problem to overwrite the user’s settings forgenerate
. Also, options that are only used in the transform phase would simply go unused, so I don’t think they would have any side effects.https://github.com/sveltejs/svelte/blob/main/packages/svelte/src/compiler/phases/3-transform/index.js#L20-L30
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.
Compiler options have many properties, and I don't think we understand how they affect linting. Also, I don't think users set compiler options with linting in mind.
Therefore, I think it's better to avoid passing them directly.
I think it's better to pass only the necessary properties that we understand.
https://svelte.dev/docs/svelte/svelte-compiler#CompileOptions
https://svelte.dev/docs/svelte/svelte-compiler#ModuleCompileOptions
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 believe I understand your concern. I have no objection to making the fix.
However, in this particular case, do we really need to worry about the affect? Wouldn’t it be fine to simply compile exactly as the user specified and then display whatever warnings are generated to the user? If something strange happens, wouldn’t that just mean the user’s configuration was wrong?
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.
The reason I'm concerned may be because I don't know the internals of the compiler, so I don't have the information to not worry 😓