Skip to content

(feat) rename of component props in other component #1043

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 2 commits into from
Jun 3, 2021
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
Original file line number Diff line number Diff line change
Expand Up @@ -136,13 +136,7 @@ export class CompletionsProviderImpl implements CompletionsProvider<CompletionEn
return null;
}

const eventCompletions = await this.getEventCompletions(
lang,
document,
tsDoc,
fragment,
position
);
const eventCompletions = await this.getEventCompletions(lang, document, tsDoc, position);

if (isEventTriggerCharacter) {
return CompletionList.create(eventCompletions, !!tsDoc.parserError);
Expand Down Expand Up @@ -214,15 +208,13 @@ export class CompletionsProviderImpl implements CompletionsProvider<CompletionEn
lang: ts.LanguageService,
doc: Document,
tsDoc: SvelteDocumentSnapshot,
fragment: SvelteSnapshotFragment,
originalPosition: Position
): Promise<Array<AppCompletionItem<CompletionEntryWithIdentifer>>> {
const snapshot = await getComponentAtPosition(
this.lsAndTsDocResolver,
lang,
doc,
tsDoc,
fragment,
originalPosition
);
if (!snapshot) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import ts from 'typescript';
import { Hover, Position } from 'vscode-languageserver';
import { Document, getWordAt, mapObjWithRangeToOriginal } from '../../../lib/documents';
import { HoverProvider } from '../../interfaces';
import { SvelteDocumentSnapshot, SvelteSnapshotFragment } from '../DocumentSnapshot';
import { SvelteDocumentSnapshot } from '../DocumentSnapshot';
import { LSAndTSDocResolver } from '../LSAndTSDocResolver';
import { getMarkdownDocumentation } from '../previewer';
import { convertRange } from '../utils';
Expand All @@ -15,13 +15,7 @@ export class HoverProviderImpl implements HoverProvider {
const { lang, tsDoc } = await this.getLSAndTSDoc(document);
const fragment = await tsDoc.getFragment();

const eventHoverInfo = await this.getEventHoverInfo(
lang,
document,
tsDoc,
fragment,
position
);
const eventHoverInfo = await this.getEventHoverInfo(lang, document, tsDoc, position);
if (eventHoverInfo) {
return eventHoverInfo;
}
Expand Down Expand Up @@ -66,7 +60,6 @@ export class HoverProviderImpl implements HoverProvider {
lang: ts.LanguageService,
doc: Document,
tsDoc: SvelteDocumentSnapshot,
fragment: SvelteSnapshotFragment,
originalPosition: Position
): Promise<Hover | null> {
const possibleEventName = getWordAt(doc.getText(), doc.offsetAt(originalPosition), {
Expand All @@ -82,7 +75,6 @@ export class HoverProviderImpl implements HoverProvider {
lang,
doc,
tsDoc,
fragment,
originalPosition
);
if (!component) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,5 @@
import { Position, WorkspaceEdit, Range } from 'vscode-languageserver';
import {
Document,
mapRangeToOriginal,
positionAt,
offsetAt,
getLineAtPosition
} from '../../../lib/documents';
import { Document, mapRangeToOriginal, getLineAtPosition } from '../../../lib/documents';
import { filterAsync, isNotNullOrUndefined, pathToUrl } from '../../../utils';
import { RenameProvider } from '../../interfaces';
import {
Expand All @@ -17,7 +11,7 @@ import { convertRange } from '../utils';
import { LSAndTSDocResolver } from '../LSAndTSDocResolver';
import ts from 'typescript';
import { uniqWith, isEqual } from 'lodash';
import { isNoTextSpanInGeneratedCode, SnapshotFragmentMap } from './utils';
import { isComponentAtPosition, isNoTextSpanInGeneratedCode, SnapshotFragmentMap } from './utils';

export class RenameProviderImpl implements RenameProvider {
constructor(private readonly lsAndTsDocResolver: LSAndTSDocResolver) {}
Expand All @@ -29,7 +23,7 @@ export class RenameProviderImpl implements RenameProvider {
const fragment = await tsDoc.getFragment();

const offset = fragment.offsetAt(fragment.getGeneratedPosition(position));
const renameInfo = this.getRenameInfo(lang, tsDoc, offset);
const renameInfo = this.getRenameInfo(lang, tsDoc, document, position, offset);
if (!renameInfo) {
return null;
}
Expand All @@ -47,7 +41,7 @@ export class RenameProviderImpl implements RenameProvider {

const offset = fragment.offsetAt(fragment.getGeneratedPosition(position));

if (!this.getRenameInfo(lang, tsDoc, offset)) {
if (!this.getRenameInfo(lang, tsDoc, document, position, offset)) {
return null;
}

Expand Down Expand Up @@ -118,7 +112,9 @@ export class RenameProviderImpl implements RenameProvider {
private getRenameInfo(
lang: ts.LanguageService,
tsDoc: SvelteDocumentSnapshot,
offset: number
doc: Document,
originalPosition: Position,
generatedOffset: number
): {
canRename: true;
kind: ts.ScriptElementKind;
Expand All @@ -131,20 +127,18 @@ export class RenameProviderImpl implements RenameProvider {
if (tsDoc.parserError) {
return null;
}
const renameInfo: any = lang.getRenameInfo(tsDoc.filePath, offset, {
const renameInfo: any = lang.getRenameInfo(tsDoc.filePath, generatedOffset, {
allowRenameOfImportPath: false
});
// TODO this will also forbid renames of svelte component properties
// in another component because the ScriptElementKind is a JSXAttribute.
// To fix this we would need to enhance svelte2tsx with info methods like
// "what props does this file have?"
if (
!renameInfo.canRename ||
renameInfo.kind === ts.ScriptElementKind.jsxAttribute ||
renameInfo.fullDisplayName?.includes('JSX.IntrinsicElements')
renameInfo.fullDisplayName?.includes('JSX.IntrinsicElements') ||
(renameInfo.kind === ts.ScriptElementKind.jsxAttribute &&
!isComponentAtPosition(doc, tsDoc, originalPosition))
) {
return null;
}

return renameInfo;
}

Expand Down Expand Up @@ -279,13 +273,9 @@ export class RenameProviderImpl implements RenameProvider {

// --------> svelte2tsx?
private isInSvelte2TsxPropLine(fragment: SvelteSnapshotFragment, loc: ts.RenameLocation) {
const pos = positionAt(loc.textSpan.start, fragment.text);
const textInLine = fragment.text.substring(
offsetAt({ ...pos, character: 0 }, fragment.text),
loc.textSpan.start
);
const textBeforeProp = fragment.text.substring(0, loc.textSpan.start);
// This is how svelte2tsx writes out the props
if (textInLine.includes('return { props: {')) {
if (textBeforeProp.includes('\nreturn { props: {')) {
return true;
}
}
Expand Down
29 changes: 22 additions & 7 deletions packages/language-server/src/plugins/typescript/features/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,7 @@ import {
getNodeIfIsInComponentStartTag,
isInTag
} from '../../../lib/documents';
import {
DocumentSnapshot,
SnapshotFragment,
SvelteDocumentSnapshot,
SvelteSnapshotFragment
} from '../DocumentSnapshot';
import { DocumentSnapshot, SnapshotFragment, SvelteDocumentSnapshot } from '../DocumentSnapshot';
import { LSAndTSDocResolver } from '../LSAndTSDocResolver';

/**
Expand All @@ -23,7 +18,6 @@ export async function getComponentAtPosition(
lang: ts.LanguageService,
doc: Document,
tsDoc: SvelteDocumentSnapshot,
fragment: SvelteSnapshotFragment,
originalPosition: Position
): Promise<SvelteDocumentSnapshot | null> {
if (tsDoc.parserError) {
Expand All @@ -43,6 +37,7 @@ export async function getComponentAtPosition(
return null;
}

const fragment = await tsDoc.getFragment();
const generatedPosition = fragment.getGeneratedPosition(doc.positionAt(node.start + 1));
const def = lang.getDefinitionAtPosition(
tsDoc.filePath,
Expand All @@ -59,6 +54,26 @@ export async function getComponentAtPosition(
return snapshot;
}

export function isComponentAtPosition(
doc: Document,
tsDoc: SvelteDocumentSnapshot,
originalPosition: Position
): boolean {
if (tsDoc.parserError) {
return false;
}

if (
isInTag(originalPosition, doc.scriptInfo) ||
isInTag(originalPosition, doc.moduleScriptInfo)
) {
// Inside script tags -> not a component
return false;
}

return !!getNodeIfIsInComponentStartTag(doc.html, doc.offsetAt(originalPosition));
}

/**
* Checks if this a section that should be completely ignored
* because it's purely generated.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,13 +209,21 @@ describe('RenameProvider', () => {
assert.deepStrictEqual(result, expectedEditsForPropRename);
});

// TODO this does not work right now, see `RenameProviderImpl.cannotRename` for more explanation
// it('should do rename of prop of component A in component B', async () => {
// const { provider, renameDoc2 } = await setup();
// const result = await provider.rename(renameDoc2, Position.create(4, 10), 'newName');
it('should do rename of prop of component A in component B', async () => {
const { provider, renameDoc2 } = await setup();
const result = await provider.rename(renameDoc2, Position.create(5, 10), 'newName');

// assert.deepStrictEqual(result, expectedEditsForPropRename);
// });
assert.deepStrictEqual(result, expectedEditsForPropRename);
});

it('should not allow rename of intrinsic attribute', async () => {
const { provider, renameDoc2 } = await setup();
const prepareResult = await provider.prepareRename(renameDoc2, Position.create(7, 7));
const renameResult = await provider.rename(renameDoc2, Position.create(7, 7), 'newName');

assert.deepStrictEqual(prepareResult, null);
assert.deepStrictEqual(renameResult, null);
});

it('should do rename of prop without type of component A in component A', async () => {
const { provider, renameDoc3 } = await setup();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@ import Rename3 from './rename3.svelte';
</script>

<Rename exportedProp={2}></Rename>
<Rename3 exportedPropFromJs={2}></Rename3>
<Rename3 exportedPropFromJs={2}></Rename3>
<div class="foo" />