Skip to content

Add support for store access type infomation #192

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

Merged
merged 4 commits into from
Jul 25, 2022
Merged
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
2 changes: 1 addition & 1 deletion .github/workflows/NodeCI.yml
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ jobs:
node-version: ${{ matrix.node-version }}
- name: Install @typescript-eslint v4
run: |+
yarn add -D @typescript-eslint/parser@4 @typescript-eslint/eslint-plugin@4 --ignore-engines
yarn add -D @typescript-eslint/parser@4 @typescript-eslint/eslint-plugin@4 eslint@7 --ignore-engines
rm -rf node_modules
- name: Install Packages
run: yarn install --ignore-engines
Expand Down
59 changes: 50 additions & 9 deletions src/context/script-let.ts
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,26 @@ export class ScriptLetContext {
this.closeScopeCallbacks.pop()!();
}

public appendDeclareMaybeStores(maybeStores: Set<string>): void {
const reservedNames = new Set<string>([
"$$props",
"$$restProps",
"$$slots",
]);
for (const nm of maybeStores) {
if (reservedNames.has(nm)) continue;

this.appendScriptWithoutOffset(
`declare let $${nm}: Parameters<Parameters<(typeof ${nm})["subscribe"]>[0]>[0];`,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if there is already an identifier named $nm or it's already declared by the user manually?

This could be uncommon, but before this PR this could happen if the user want to workaround this issue, then this PR would break that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the edge case and it didn't seem to be a problem.
https://github.com/ota-meshi/svelte-eslint-parser/blob/store-type/tests/fixtures/parser/ast/ts-store01-type-output.svelte

Also, the svelte compiler doesn't seem to allow you to declare variables that start with $ yourself.

image

<script>
	let name = 'world';
	const $n = name
</script>

<h1>Hello {name}!</h1>

Copy link
Collaborator

@JounQin JounQin Jul 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does script with lang="ts" + user defined declare const $n: xxx works the same?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test case to check that edge case. That doesn't seem to be a problem either 😉.

https://github.com/ota-meshi/svelte-eslint-parser/blob/store-type/tests/fixtures/parser/ast/ts-store02-type-output.svelte

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, it should due to

tokens.length = 0
comments.length = 0
removeAllScope(node, result)

I just learned a lot from this PR!

(node, tokens, comments, result) => {
tokens.length = 0;
comments.length = 0;
removeAllScope(node, result);
}
);
}
}

private appendScript(
text: string,
offset: number,
Expand Down Expand Up @@ -912,7 +932,7 @@ function removeAllScope(target: ESTree.Node, result: ScriptLetCallbackOption) {
return;
}
if (node.type === "Identifier") {
let scope = result.getScope(node);
let scope = result.getInnermostScope(node);
if (
(scope.block as any).type === "TSTypeAliasDeclaration" &&
(scope.block as any).id === node
Expand All @@ -939,16 +959,37 @@ function removeAllScope(target: ESTree.Node, result: ScriptLetCallbackOption) {

/** Remove variable */
function removeIdentifierVariable(node: ESTree.Identifier, scope: Scope): void {
const varIndex = scope.variables.findIndex((v) =>
v.defs.some((def) => def.name === node)
);
if (varIndex >= 0) {
for (let varIndex = 0; varIndex < scope.variables.length; varIndex++) {
const variable = scope.variables[varIndex];
scope.variables.splice(varIndex, 1);
const name = node.name;
if (variable === scope.set.get(name)) {
scope.set.delete(name);
const defIndex = variable.defs.findIndex((def) => def.name === node);
if (defIndex < 0) {
continue;
}
variable.defs.splice(defIndex, 1);
if (variable.defs.length === 0) {
// Remove variable
referencesToThrough(variable.references, scope);
scope.variables.splice(varIndex, 1);
const name = node.name;
if (variable === scope.set.get(name)) {
scope.set.delete(name);
}
} else {
const idIndex = variable.identifiers.indexOf(node);
if (idIndex >= 0) {
variable.identifiers.splice(idIndex, 1);
}
}
return;
}
}

/** Move reference to through */
function referencesToThrough(references: Reference[], baseScope: Scope) {
let scope: Scope | null = baseScope;
while (scope) {
scope.through.push(...references);
scope = scope.upper;
}
}

Expand Down
21 changes: 21 additions & 0 deletions src/parser/analyze-type/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import type { Context } from "../../context";

/**
* Append store type declarations.
* Append TypeScript code like
* `declare let $foo: Parameters<Parameters<(typeof foo)["subscribe"]>[0]>[0];`
* to define the type information for like $foo variable.
*/
export function appendDeclareStoreTypes(ctx: Context): void {
const vcode = ctx.sourceCode.scripts.vcode;
const extractStoreRe = /\$[\p{ID_Start}$_][\p{ID_Continue}$\u200c\u200d]*/giu;
let m;
const maybeStores = new Set<string>();
while ((m = extractStoreRe.exec(vcode))) {
const storeName = m[0];
const originalName = storeName.slice(1);
maybeStores.add(originalName);
}

ctx.scriptLet.appendDeclareMaybeStores(maybeStores);
}
3 changes: 3 additions & 0 deletions src/parser/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
analyzeStoreScope,
} from "./analyze-scope";
import { ParseError } from "../errors";
import { appendDeclareStoreTypes } from "./analyze-type";

export interface ESLintProgram extends Program {
comments: Comment[];
Expand Down Expand Up @@ -76,6 +77,8 @@ export function parseForESLint(
parserOptions
);

if (ctx.isTypeScript()) appendDeclareStoreTypes(ctx);

const resultScript = parseScript(ctx.sourceCode.scripts, parserOptions);
ctx.scriptLet.restore(resultScript);
ctx.tokens.push(...resultScript.ast.tokens);
Expand Down
7 changes: 7 additions & 0 deletions tests/fixtures/integrations/type-info-tests/i18n-input.svelte
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<script lang="ts">
import { _ } from './svelte-i18n';
</script>

<main>
<input name={$_('test')}>
</main>
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
[]
24 changes: 24 additions & 0 deletions tests/fixtures/integrations/type-info-tests/i18n-setup.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/* eslint eslint-comments/require-description: 0, @typescript-eslint/explicit-module-boundary-types: 0 */
import type { Linter } from "eslint";
import { BASIC_PARSER_OPTIONS } from "../../../src/parser/test-utils";
import { rules } from "@typescript-eslint/eslint-plugin";
export function setupLinter(linter: Linter) {
linter.defineRule(
"@typescript-eslint/no-unsafe-call",
rules["no-unsafe-call"] as never
);
}

export function getConfig() {
return {
parser: "svelte-eslint-parser",
parserOptions: BASIC_PARSER_OPTIONS,
rules: {
"@typescript-eslint/no-unsafe-call": "error",
},
env: {
browser: true,
es2021: true,
},
};
}
6 changes: 6 additions & 0 deletions tests/fixtures/integrations/type-info-tests/svelte-i18n.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import type { Readable } from "svelte/store";

declare type MessageFormatter = (id: string) => string;
declare const $format: Readable<MessageFormatter>;

export { $format as _ };
7 changes: 7 additions & 0 deletions tests/fixtures/parser/ast/i18n-test-input.svelte
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<script lang="ts">
import { _ } from './i18n-test-svelte-i18n';
</script>

<main>
<input name={$_('test')}>
</main>
Loading