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(no-shadow): ignore {#snippet} if it uses under component #992

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/silly-lions-build.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'eslint-plugin-svelte': patch
---

fix(no-shadow): ignore `{#snippet}` if it uses under component.
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,7 @@ These rules relate to better ways of doing things to help you avoid problems:
| [svelte/no-inspect](https://sveltejs.github.io/eslint-plugin-svelte/rules/no-inspect/) | Warns against the use of `$inspect` directive | |
| [svelte/no-reactive-functions](https://sveltejs.github.io/eslint-plugin-svelte/rules/no-reactive-functions/) | it's not necessary to define functions in reactive statements | :bulb: |
| [svelte/no-reactive-literals](https://sveltejs.github.io/eslint-plugin-svelte/rules/no-reactive-literals/) | don't assign literal values in reactive statements | :bulb: |
| [svelte/no-shadow](https://sveltejs.github.io/eslint-plugin-svelte/rules/no-shadow/) | Disallow variable declarations from shadowing variables declared in the outer scope | |
| [svelte/no-svelte-internal](https://sveltejs.github.io/eslint-plugin-svelte/rules/no-svelte-internal/) | svelte/internal will be removed in Svelte 6. | |
| [svelte/no-unused-class-name](https://sveltejs.github.io/eslint-plugin-svelte/rules/no-unused-class-name/) | disallow the use of a class in the template without a corresponding style | |
| [svelte/no-unused-svelte-ignore](https://sveltejs.github.io/eslint-plugin-svelte/rules/no-unused-svelte-ignore/) | disallow unused svelte-ignore comments | :star: |
Expand Down
1 change: 1 addition & 0 deletions docs/rules.md
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ These rules relate to better ways of doing things to help you avoid problems:
| [svelte/no-inspect](./rules/no-inspect.md) | Warns against the use of `$inspect` directive | |
| [svelte/no-reactive-functions](./rules/no-reactive-functions.md) | it's not necessary to define functions in reactive statements | :bulb: |
| [svelte/no-reactive-literals](./rules/no-reactive-literals.md) | don't assign literal values in reactive statements | :bulb: |
| [svelte/no-shadow](./rules/no-shadow.md) | Disallow variable declarations from shadowing variables declared in the outer scope | |
| [svelte/no-svelte-internal](./rules/no-svelte-internal.md) | svelte/internal will be removed in Svelte 6. | |
| [svelte/no-unused-class-name](./rules/no-unused-class-name.md) | disallow the use of a class in the template without a corresponding style | |
| [svelte/no-unused-svelte-ignore](./rules/no-unused-svelte-ignore.md) | disallow unused svelte-ignore comments | :star: |
Expand Down
74 changes: 74 additions & 0 deletions docs/rules/no-shadow.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
---
pageClass: 'rule-details'
sidebarDepth: 0
title: 'svelte/no-shadow'
description: 'Disallow variable declarations from shadowing variables declared in the outer scope'
---

# svelte/no-shadow

> Disallow variable declarations from shadowing variables declared in the outer scope

- :exclamation: <badge text="This rule has not been released yet." vertical="middle" type="error"> **_This rule has not been released yet._** </badge>

## :book: Rule Details

This rule reports shadowed variables, similar to the base ESLint `no-shadow` rule. However, it ignores cases where `{#snippet}` is used as a named slot in Svelte components. If this rule is active, make sure to disable the base `no-shadow` rule, as it will conflict with this rule.

<!--eslint-skip-->

```svelte
<script>
/* eslint svelte/no-shadow: "error" */
import ComponentWithSnippet from './ComponentWithSnippet.svelte';
</script>

<!-- ✓ GOOD -->
<ComponentWithSnippet>
{#snippet children()}
<AnotherComponentWithSnippet>
{#snippet children()}
Hello!
{/snippet}
</AnotherComponentWithSnippet>
{/snippet}
</ComponentWithSnippet>

<!-- ✗ BAD -->
<ComponentWithSnippet>
{@const foo = 1}
<ComponentWithSnippet>
{@const foo = 2}
</ComponentWithSnippet>
</ComponentWithSnippet>
```

## :wrench: Options

```json
{
"svelte/no-shadow": [
"error",
{ "builtinGlobals": false, "hoist": "functions", "allow": [], "ignoreOnInitialization": false }
]
}
```

- `builtinGlobals`: The `builtinGlobals` option is `false` by default. If it is `true`, the rule prevents shadowing of built-in global variables: `Object`, `Array`, `Number`, and so on.
- `hoist`: The `hoist` option has three settings:
- `functions` (by default) - reports shadowing before the outer functions are defined.
- `all` - reports all shadowing before the outer variables/functions are defined.
- `never` - never report shadowing before the outer variables/functions are defined.
- `allow`: The `allow` option is an array of identifier names for which shadowing is allowed. For example, `"resolve"`, `"reject"`, `"done"`, `"cb"`.
- `ignoreOnInitialization`: The `ignoreOnInitialization` option is `false` by default. If it is `true`, it prevents reporting shadowing of variables in their initializers when the shadowed variable is presumably still uninitialized. The shadowed variable must be on the left side. The shadowing variable must be on the right side and declared in a callback function or in an IIFE.

## :books: Further Reading

- See [ESLint `no-shadow` rule](https://eslint.org/docs/latest/rules/no-shadow) for more information about the base rule.

## :mag: Implementation

- [Rule source](https://github.com/sveltejs/eslint-plugin-svelte/blob/main/packages/eslint-plugin-svelte/src/rules/no-shadow.ts)
- [Test source](https://github.com/sveltejs/eslint-plugin-svelte/blob/main/packages/eslint-plugin-svelte/tests/src/rules/no-shadow.ts)

<sup>Taken with ❤️ [from ESLint core](https://eslint.org/docs/rules/no-shadow)</sup>
12 changes: 12 additions & 0 deletions packages/eslint-plugin-svelte/src/rule-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,11 @@ export interface RuleOptions {
* @see https://sveltejs.github.io/eslint-plugin-svelte/rules/no-restricted-html-elements/
*/
'svelte/no-restricted-html-elements'?: Linter.RuleEntry<SvelteNoRestrictedHtmlElements>
/**
* Disallow variable declarations from shadowing variables declared in the outer scope
* @see https://sveltejs.github.io/eslint-plugin-svelte/rules/no-shadow/
*/
'svelte/no-shadow'?: Linter.RuleEntry<SvelteNoShadow>
/**
* disallow shorthand style properties that override related longhand properties
* @see https://sveltejs.github.io/eslint-plugin-svelte/rules/no-shorthand-style-property-overrides/
Expand Down Expand Up @@ -479,6 +484,13 @@ type SvelteNoRestrictedHtmlElements = [(string | {
elements?: [string, ...(string)[]]
message?: string
}))[]]
// ----- svelte/no-shadow -----
type SvelteNoShadow = []|[{
builtinGlobals?: boolean
hoist?: ("all" | "functions" | "never")
allow?: string[]
ignoreOnInitialization?: boolean
}]
// ----- svelte/no-target-blank -----
type SvelteNoTargetBlank = []|[{
allowReferrer?: boolean
Expand Down
83 changes: 83 additions & 0 deletions packages/eslint-plugin-svelte/src/rules/no-shadow.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
import { createRule } from '../utils/index.js';
import { defineWrapperListener, getProxyContent, getCoreRule } from '../utils/eslint-core.js';
import type { TSESTree } from '@typescript-eslint/types';
import type { Scope } from '@typescript-eslint/scope-manager';
import type { Range } from 'svelte-eslint-parser/lib/ast/common.js';
import { getScope as getScopeUtil } from '../utils/ast-utils.js';
import { getSourceCode as getSourceCodeCompat } from '../utils/compat.js';

const coreRule = getCoreRule('no-shadow');

function removeSnippetIdentifiers(snippetIdentifierNodeLocations: Range[], scope: Scope): Scope {
return {
...scope,
variables: scope.variables.filter((variable) => {
return !snippetIdentifierNodeLocations.some(([start, end]) => {
return variable.identifiers.every((identifier) => {
const { range } = identifier;
return range[0] === start && range[1] === end;
});
});
}),
childScopes: scope.childScopes.map((scope) => {
return removeSnippetIdentifiers(snippetIdentifierNodeLocations, scope);
})
} as Scope;
}

export default createRule('no-shadow', {
meta: {
...coreRule.meta,
docs: {
description: coreRule.meta.docs.description,
category: 'Best Practices',
recommended: false,
extensionRule: 'no-shadow'
}
},
create(context) {
const snippetIdentifierNodeLocations: Range[] = [];

function getScope(node: TSESTree.Node) {
const scope = getScopeUtil(context, node);
return removeSnippetIdentifiers(snippetIdentifierNodeLocations, scope);
}

function getSourceCode() {
const sourceCode = getSourceCodeCompat(context);
return new Proxy(sourceCode, {
get(target, key) {
if (key === 'getScope') {
return getScope;
}
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- ignore
return (target as any)[key];
}
});
}

return defineWrapperListener(
coreRule,
getProxyContent(context, {
sourceCode: getSourceCode()
}),
{
createListenerProxy(coreListener) {
return {
...coreListener,
SvelteSnippetBlock(node) {
const parent = node.parent;
if (parent.type === 'SvelteElement' && parent.kind === 'component') {
snippetIdentifierNodeLocations.push(node.id.range);
}
coreListener.SvelteSnippetBlock?.(node);
},
'Program:exit'(node) {
coreListener['Program:exit']?.(node);
}
};
}
}
);
}
});
21 changes: 17 additions & 4 deletions packages/eslint-plugin-svelte/src/utils/eslint-core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,22 @@ import { Linter } from 'eslint';
import Module from 'module';

const require = Module.createRequire(import.meta.url);

export function getProxyContent(context: RuleContext, overrides: any): RuleContext {
const cache: any = {};
return new Proxy(context, {
get(_t, key) {
if (key in cache) {
return cache[key];
}
if (key in overrides) {
return (cache[key] = overrides[key]);
}
return (context as any)[key];
}
});
}

/**
* Define the wrapped core rule.
*/
Expand All @@ -17,10 +33,7 @@ export function defineWrapperListener(
}
): RuleListener {
const listener = coreRule.create(context as any);

const svelteListener = proxyOptions.createListenerProxy?.(listener) ?? listener;

return svelteListener;
return proxyOptions.createListenerProxy?.(listener) ?? listener;
}

/**
Expand Down
2 changes: 2 additions & 0 deletions packages/eslint-plugin-svelte/src/utils/rules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import noReactiveFunctions from '../rules/no-reactive-functions.js';
import noReactiveLiterals from '../rules/no-reactive-literals.js';
import noReactiveReassign from '../rules/no-reactive-reassign.js';
import noRestrictedHtmlElements from '../rules/no-restricted-html-elements.js';
import noShadow from '../rules/no-shadow.js';
import noShorthandStylePropertyOverrides from '../rules/no-shorthand-style-property-overrides.js';
import noSpacesAroundEqualSignsInAttribute from '../rules/no-spaces-around-equal-signs-in-attribute.js';
import noStoreAsync from '../rules/no-store-async.js';
Expand Down Expand Up @@ -113,6 +114,7 @@ export const rules = [
noReactiveLiterals,
noReactiveReassign,
noRestrictedHtmlElements,
noShadow,
noShorthandStylePropertyOverrides,
noSpacesAroundEqualSignsInAttribute,
noStoreAsync,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
- message: "'x' is already declared in the upper scope on line 2 column 13."
line: 4
column: 8
suggestions: null
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<script>
function a(x) {
var b = function c() {
var x = 'foo';
};
}
</script>
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
- message: "'foo' is already declared in the upper scope on line 6 column 10."
line: 8
column: 11
suggestions: null
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<script>
import ComponentWithSnippet from './ComponentWithSnippet.svelte';
</script>

<ComponentWithSnippet>
{@const foo = 1}
<ComponentWithSnippet>
{@const foo = 2}
</ComponentWithSnippet>
</ComponentWithSnippet>
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<script>
var a = 3;
var b = (x) => {
a++;
return x + a;
};
setTimeout(() => {
b(a);
}, 0);
</script>
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<script>
import ComponentWithSnippet from './ComponentWithSnippet.svelte';
</script>

<ComponentWithSnippet>
{#snippet children()}
<AnotherComponentWithSnippet>
{#snippet children()}
Hello!
{/snippet}
</AnotherComponentWithSnippet>
{/snippet}
</ComponentWithSnippet>
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"svelte": ">=5.0.0-0"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<script>
import ComponentWithSnippet from './ComponentWithSnippet.svelte';
const children = 1;
</script>

<ComponentWithSnippet>
{#snippet children()}
Hello!
{/snippet}
</ComponentWithSnippet>
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"svelte": ">=5.0.0-0"
}
12 changes: 12 additions & 0 deletions packages/eslint-plugin-svelte/tests/src/rules/no-shadow.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import { RuleTester } from '../../utils/eslint-compat.js';
import rule from '../../../src/rules/no-shadow.js';
import { loadTestCases } from '../../utils/utils.js';

const tester = new RuleTester({
languageOptions: {
ecmaVersion: 2020,
sourceType: 'module'
}
});

tester.run('no-shadow', rule as any, loadTestCases('no-shadow'));
Loading