Skip to content

Commit

Permalink
fix: rework bindable types strategy
Browse files Browse the repository at this point in the history
Instead of using types that declare whether or not a type is bindable directly as part of the property, we're introducing a new for-types-only field to `SvelteComponent`: `$$bindings`, which is typed as the keys of the properties that are bindable (string by default, i.e. everything's bindable; for backwards compat). language-tools can then produce code that assigns to this property which results in an error we can display if the binding is invalid.
This means we can revert a lot of the changes we made to make the previous version of bindable types work
  • Loading branch information
dummdidumm committed May 1, 2024
1 parent 6cfb0d2 commit 8aa0a6e
Show file tree
Hide file tree
Showing 28 changed files with 142 additions and 687 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -145,28 +145,77 @@ export class DiagnosticsProviderImpl implements DiagnosticsProvider {

diagnostics = resolveNoopsInReactiveStatements(lang, diagnostics);

return diagnostics
.map<Diagnostic>((diagnostic) => ({
range: convertRange(tsDoc, diagnostic),
severity: mapSeverity(diagnostic.category),
const mapRange = rangeMapper(tsDoc, document, lang);
const noFalsePositive = isNoFalsePositive(document, tsDoc);
const converted: Diagnostic[] = [];

for (const tsDiag of diagnostics) {
let diagnostic: Diagnostic = {
range: convertRange(tsDoc, tsDiag),
severity: mapSeverity(tsDiag.category),
source: isTypescript ? 'ts' : 'js',
message: ts.flattenDiagnosticMessageText(diagnostic.messageText, '\n'),
code: diagnostic.code,
tags: getDiagnosticTag(diagnostic)
}))
.map(mapRange(tsDoc, document, lang))
.filter(hasNoNegativeLines)
.filter(isNoFalsePositive(document, tsDoc))
.map(adjustIfNecessary)
.map(swapDiagRangeStartEndIfNecessary);
message: ts.flattenDiagnosticMessageText(tsDiag.messageText, '\n'),
code: tsDiag.code,
tags: getDiagnosticTag(tsDiag)
};
diagnostic = mapRange(diagnostic);

moveBindingErrorMessage(tsDiag, tsDoc, diagnostic, document);

if (!hasNoNegativeLines(diagnostic) || !noFalsePositive(diagnostic)) {
continue;
}

diagnostic = adjustIfNecessary(diagnostic);
diagnostic = swapDiagRangeStartEndIfNecessary(diagnostic);
converted.push(diagnostic);
}

return converted;
}

private async getLSAndTSDoc(document: Document) {
return this.lsAndTsDocResolver.getLSAndTSDoc(document);
}
}

function mapRange(
function moveBindingErrorMessage(
tsDiag: ts.Diagnostic,
tsDoc: SvelteDocumentSnapshot,
diagnostic: Diagnostic,
document: Document
) {
if (
tsDiag.code === DiagnosticCode.TYPE_X_NOT_ASSIGNABLE_TO_TYPE_Y &&
tsDiag.start &&
tsDoc.getText(tsDiag.start, tsDiag.start + tsDiag.length!).endsWith('.$$bindings')
) {
let node = tsDoc.svelteNodeAt(diagnostic.range.start);
while (node && node.type !== 'InlineComponent') {
node = node.parent!;
}
if (node) {
let name = tsDoc.getText(
tsDiag.start + tsDiag.length!,
tsDiag.start + tsDiag.length! + 100
);
const quoteIdx = name.indexOf("'");
name = name.substring(quoteIdx + 1, name.indexOf("'", quoteIdx + 1));
const binding: any = node.attributes.find(
(attr: any) => attr.type === 'Binding' && attr.name === name
);
if (binding) {
diagnostic.message = 'Cannot bind: to this property\n\n' + diagnostic.message;
diagnostic.range = {
start: document.positionAt(binding.start),
end: document.positionAt(binding.end)
};
}
}
}
}

function rangeMapper(
snapshot: SvelteDocumentSnapshot,
document: Document,
lang: ts.LanguageService
Expand Down Expand Up @@ -194,7 +243,7 @@ function mapRange(
diagnostic.code as number
) ||
(DiagnosticCode.TYPE_X_NOT_ASSIGNABLE_TO_TYPE_Y &&
diagnostic.message.includes("'PropsWithChildren<"))) &&
diagnostic.message.includes("'Properties<"))) &&
!hasNonZeroRange({ range })
) {
const node = getNodeIfIsInStartTag(document.html, document.offsetAt(range.start));
Expand Down Expand Up @@ -327,37 +376,6 @@ function adjustIfNecessary(diagnostic: Diagnostic): Diagnostic {
};
}

if (
(diagnostic.code === DiagnosticCode.TYPE_X_NOT_ASSIGNABLE_TO_TYPE_Y ||
diagnostic.code === DiagnosticCode.TYPE_X_NOT_ASSIGNABLE_TO_TYPE_Y_DID_YOU_MEAN) &&
diagnostic.message.includes("'Bindable<")
) {
const countBindable = (diagnostic.message.match(/'Bindable\</g) || []).length;
const countBinding = (diagnostic.message.match(/'Binding\</g) || []).length;
if (countBindable === 1 && countBinding === 0) {
// Remove distracting Bindable<...> from diagnostic message
const start = diagnostic.message.indexOf("'Bindable<");
const startType = start + "'Bindable".length;
const end = traverseTypeString(diagnostic.message, startType, '>');
diagnostic.message =
diagnostic.message.substring(0, start + 1) +
diagnostic.message.substring(startType + 1, end) +
diagnostic.message.substring(end + 1);
} else if (countBinding === 3 && countBindable === 1) {
// Only keep Type '...' is not assignable to type '...' in
// Type Bindings<...> is not assignable to type Bindable<...>, Type Binding<...> is not assignable to type Bindable<...>, Type '...' is not assignable to type '...'
const lines = diagnostic.message.split('\n');
if (lines.length === 3) {
diagnostic.message = lines[2].trimStart();
}
}

return {
...diagnostic,
message: diagnostic.message
};
}

return diagnostic;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -638,38 +638,6 @@ export class RenameProviderImpl implements RenameProvider {
suffixText: '}'
};
}

if (snapshot.isSvelte5Plus) {
const bindingShorthand = this.getBindingShorthand(
snapshot,
location.range.start
);
if (bindingShorthand) {
const name = parent
.getText()
.substring(bindingShorthand.start, bindingShorthand.end);
const start = {
line: location.range.start.line,
character: location.range.start.character - name.length
};
// If binding is followed by the closing tag, start is one character too soon,
// else binding is ending one character too far
if (parent.getText().charAt(parent.offsetAt(start)) === ':') {
start.character++;
} else {
location.range.end.character--;
}
return {
...location,
range: {
start: start,
end: location.range.end
},
prefixText: name + '={',
suffixText: '}'
};
}
}
}

if (!prefixText || prefixText.slice(-1) !== ':') {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ describe('Svelte Plugin', () => {
const diagnostic = Diagnostic.create(
Range.create(1, 0, 1, 21),
isSvelte5Plus
? '<img> element should have an alt attribute'
? '`<img>` element should have an alt attribute'
: 'A11y: <img> element should have an alt attribute',
DiagnosticSeverity.Warning,
isSvelte5Plus ? 'a11y_missing_attribute' : 'a11y-missing-attribute',
Expand All @@ -57,7 +57,7 @@ describe('Svelte Plugin', () => {
Range.create(0, 10, 0, 18),
isSvelte5Plus ? 'Can only bind to state or props' : 'whatever is not declared',
DiagnosticSeverity.Error,
isSvelte5Plus ? 'invalid_binding_value' : 'binding-undeclared',
isSvelte5Plus ? 'bind_invalid_value' : 'binding-undeclared',
'svelte'
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,7 @@ describe('SveltePlugin#getDiagnostics', () => {
"Component has unused export property 'name'. If it is for external reference only, please consider using `export const name`",
severity: 2,
source: 'svelte',
code: isSvelte5Plus ? 'unused_export_let' : 'unused-export-let'
code: isSvelte5Plus ? 'export_let_unused' : 'unused-export-let'
}
]);
});
Expand All @@ -477,7 +477,7 @@ describe('SveltePlugin#getDiagnostics', () => {
severity: 2,
source: 'svelte',
code: isSvelte5Plus
? 'no_reactive_declaration'
? 'reactive_declaration_invalid_placement'
: 'module-script-reactive-declaration'
}
]);
Expand All @@ -489,7 +489,7 @@ describe('SveltePlugin#getDiagnostics', () => {

assert.deepStrictEqual(diagnostics, [
{
code: isSvelte5Plus ? 'unused_export_let' : 'unused-export-let',
code: isSvelte5Plus ? 'export_let_unused' : 'unused-export-let',
message:
"Component has unused export property 'unused1'. If it is for external reference only, please consider using `export const unused1`",
range: {
Expand All @@ -506,7 +506,7 @@ describe('SveltePlugin#getDiagnostics', () => {
source: 'svelte'
},
{
code: isSvelte5Plus ? 'unused_export_let' : 'unused-export-let',
code: isSvelte5Plus ? 'export_let_unused' : 'unused-export-let',
message:
"Component has unused export property 'unused2'. If it is for external reference only, please consider using `export const unused2`",
range: {
Expand Down

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
},
"severity": 1,
"source": "ts",
"message": "Type 'Component' is not assignable to type 'OtherComponent'.\n The types of '$$prop_def.prop' are incompatible between these types.\n Type 'boolean' is not assignable to type 'string'.",
"message": "Type 'Component' is not assignable to type 'OtherComponent'.\n Types of property '$$prop_def' are incompatible.\n Type '{ prop: boolean; }' is not assignable to type '{ prop: string; }'.",
"code": 2322,
"tags": []
},
Expand All @@ -58,7 +58,7 @@
},
"severity": 1,
"source": "ts",
"message": "Type '{}' is not assignable to type 'PropsWithChildren<{ prop: boolean; }, any> | undefined'.",
"message": "Type '{}' is not assignable to type 'Properties<{ prop: boolean; }, any> | undefined'.",
"code": 2322,
"tags": []
},
Expand Down Expand Up @@ -91,7 +91,7 @@
},
"severity": 1,
"source": "ts",
"message": "Type '{}' is not assignable to type 'PropsWithChildren<{ prop: boolean; }, any> | undefined'.",
"message": "Type '{}' is not assignable to type 'Properties<{ prop: boolean; }, any> | undefined'.",
"code": 2322,
"tags": []
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
[
{
"code": 2322,
"message": "Type 'Binding<string>' is not assignable to type 'string'.",
"message": "Cannot bind: to this property\n\nType '\"readonly\"' is not assignable to type '\"can_bind\"'.",
"range": {
"end": {
"character": 20,
"line": 25
},
"start": {
"character": 12,
"character": 7,
"line": 25
}
},
Expand All @@ -18,7 +18,7 @@
},
{
"code": 2353,
"message": "Object literal may only specify known properties, and 'only_bind' does not exist in type '{ readonly?: string | undefined; can_bind?: Bindable<string | undefined>; }'.",
"message": "Object literal may only specify known properties, and 'only_bind' does not exist in type '$$ComponentProps'.",
"range": {
"end": {
"character": 21,
Expand All @@ -33,9 +33,26 @@
"source": "ts",
"tags": []
},
{
"code": 2322,
"message": "Cannot bind: to this property\n\nType '\"only_bind\"' is not assignable to type '\"can_bind\"'.",
"range": {
"end": {
"character": 21,
"line": 26
},
"start": {
"character": 7,
"line": 26
}
},
"severity": 1,
"source": "ts",
"tags": []
},
{
"code": 2353,
"message": "Object literal may only specify known properties, and 'only_bind' does not exist in type '{ readonly?: string | undefined; can_bind?: Bindable<string | undefined>; }'.",
"message": "Object literal may only specify known properties, and 'only_bind' does not exist in type '$$ComponentProps'.",
"range": {
"end": {
"character": 17,
Expand Down
Loading

0 comments on commit 8aa0a6e

Please sign in to comment.