Skip to content

Commit

Permalink
fix: rework bindable types strategy (#2361)
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
dummdidumm committed May 2, 2024
1 parent 6cfb0d2 commit 9d7907e
Show file tree
Hide file tree
Showing 40 changed files with 457 additions and 751 deletions.
110 changes: 64 additions & 46 deletions packages/language-server/src/plugins/typescript/features/DiagnosticsProvider.ts
View file
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
156 changes: 60 additions & 96 deletions packages/language-server/src/plugins/typescript/features/RenameProvider.ts
View file
Expand Up @@ -31,14 +31,14 @@ import {
} from './utils';
import { LSConfigManager } from '../../../ls-config';
import { isAttributeName, isEventHandler } from '../svelte-ast-utils';
import { Identifier } from 'estree';

interface TsRenameLocation extends ts.RenameLocation {
range: Range;
newName?: string;
}

const bind = 'bind:';
const bindShortHandGeneratedLength = ':__sveltets_2_binding('.length;

export class RenameProviderImpl implements RenameProvider {
constructor(
Expand Down Expand Up @@ -76,7 +76,7 @@ export class RenameProviderImpl implements RenameProvider {

const renameLocations = lang.findRenameLocations(
tsDoc.filePath,
offset + (renameInfo.bindShorthand || 0),
offset,
false,
false,
true
Expand All @@ -101,7 +101,8 @@ export class RenameProviderImpl implements RenameProvider {
convertedRenameLocations = this.checkShortHandBindingOrSlotLetLocation(
lang,
convertedRenameLocations,
docs
docs,
newName
);

const additionalRenameForPropRenameInsideComponentWithProp =
Expand Down Expand Up @@ -160,7 +161,6 @@ export class RenameProviderImpl implements RenameProvider {
):
| (ts.RenameInfoSuccess & {
isStore?: boolean;
bindShorthand?: number;
})
| null {
// Don't allow renames in error-state, because then there is no generated svelte2tsx-code
Expand All @@ -170,14 +170,6 @@ export class RenameProviderImpl implements RenameProvider {
}

const svelteNode = tsDoc.svelteNodeAt(originalPosition);

let bindOffset = 0;
const bindingShorthand = this.getBindingShorthand(tsDoc, originalPosition, svelteNode);
if (bindingShorthand) {
bindOffset = bindingShorthand.end - bindingShorthand.start;
generatedOffset += bindShortHandGeneratedLength + bindOffset;
}

const renameInfo = lang.getRenameInfo(tsDoc.filePath, generatedOffset, {
allowRenameOfImportPath: false
});
Expand Down Expand Up @@ -211,11 +203,6 @@ export class RenameProviderImpl implements RenameProvider {
}
}

if (bindOffset) {
renameInfo.triggerSpan.start -= bindShortHandGeneratedLength + bindOffset;
(renameInfo as any).bindShorthand = bindShortHandGeneratedLength + bindOffset;
}

return renameInfo;
}

Expand Down Expand Up @@ -366,48 +353,8 @@ export class RenameProviderImpl implements RenameProvider {
return location;
}

const { parent } = snapshot;

const bindingShorthand = this.getBindingShorthand(snapshot, location.range.start);
if (bindingShorthand) {
// bind:|foo| -> bind:|newName|={foo}
const name = parent
.getText()
.substring(bindingShorthand.start, bindingShorthand.end);
return {
...location,
prefixText: '',
suffixText: `={${name}}`
};
}

let rangeStart = parent.offsetAt(location.range.start);

// suffix is of the form `: oldVarName` -> hints at a shorthand
if (
!location.suffixText?.trimStart()?.startsWith(':') ||
!getNodeIfIsInStartTag(parent.html, rangeStart)
) {
return location;
}

if (snapshot.getOriginalText().charAt(rangeStart - 1) === '{') {
// {|foo|} -> |{foo|}
rangeStart--;
return {
...location,
range: {
start: parent.positionAt(rangeStart),
end: location.range.end
},
// |{foo|} -> newName=|{foo|}
newName: parent.getText(location.range),
prefixText: `${newName}={`,
suffixText: ''
};
}

return location;
const shorthandLocation = this.transformShorthand(snapshot, location, newName);
return shorthandLocation || location;
});
}

Expand Down Expand Up @@ -595,7 +542,8 @@ export class RenameProviderImpl implements RenameProvider {
private checkShortHandBindingOrSlotLetLocation(
lang: ts.LanguageService,
renameLocations: TsRenameLocation[],
snapshots: SnapshotMap
snapshots: SnapshotMap,
newName?: string
): TsRenameLocation[] {
return renameLocations.map((location) => {
const sourceFile = lang.getProgram()?.getSourceFile(location.fileName);
Expand All @@ -616,6 +564,16 @@ export class RenameProviderImpl implements RenameProvider {

const { parent } = snapshot;

if (snapshot.isSvelte5Plus && newName && location.suffixText) {
// Svelte 5 runes mode, thanks to $props(), is much easier to handle rename-wise.
// Notably, it doesn't need the "additional props rename locations" logic, because
// these renames already appear here.
const shorthandLocation = this.transformShorthand(snapshot, location, newName);
if (shorthandLocation) {
return shorthandLocation;
}
}

let rangeStart = parent.offsetAt(location.range.start);
let prefixText = location.prefixText?.trimRight();

Expand All @@ -638,38 +596,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 Expand Up @@ -714,16 +640,54 @@ export class RenameProviderImpl implements RenameProvider {
});
}

private getBindingShorthand(
private transformShorthand(
snapshot: SvelteDocumentSnapshot,
location: TsRenameLocation,
newName: string
): TsRenameLocation | undefined {
const shorthand = this.getBindingOrAttrShorthand(snapshot, location.range.start);
if (shorthand) {
if (shorthand.isBinding) {
// bind:|foo| -> bind:|newName|={foo}
return {
...location,
prefixText: '',
suffixText: `={${shorthand.id.name}}`
};
} else {
return {
...location,
range: {
// {|foo|} -> |{foo|}
start: {
line: location.range.start.line,
character: location.range.start.character - 1
},
end: location.range.end
},
// |{foo|} -> newName=|{foo|}
newName: shorthand.id.name,
prefixText: `${newName}={`,
suffixText: ''
};
}
}
}

private getBindingOrAttrShorthand(
snapshot: SvelteDocumentSnapshot,
position: Position,
svelteNode = snapshot.svelteNodeAt(position)
) {
): { id: Identifier; isBinding: boolean } | undefined {
if (
svelteNode?.parent?.type === 'Binding' &&
(svelteNode?.parent?.type === 'Binding' ||
svelteNode?.parent?.type === 'AttributeShorthand') &&
svelteNode.parent.expression.end === svelteNode.parent.end
) {
return svelteNode.parent.expression;
return {
id: svelteNode as any as Identifier,
isBinding: svelteNode.parent.type === 'Binding'
};
}
}
}

0 comments on commit 9d7907e

Please sign in to comment.