From 24fb67a5b4ec2257764ab7ee6b8476f61ce52268 Mon Sep 17 00:00:00 2001 From: Ilya Golovin Date: Thu, 1 Feb 2024 23:44:06 +0300 Subject: [PATCH 1/5] feat: skip identifiers in vue template for addDestructure code action --- typescript/src/completionsAtPosition.ts | 2 +- typescript/src/utils.ts | 10 ++++++++-- typescript/src/utils/vue/isVueFileName.ts | 1 + 3 files changed, 10 insertions(+), 3 deletions(-) create mode 100644 typescript/src/utils/vue/isVueFileName.ts diff --git a/typescript/src/completionsAtPosition.ts b/typescript/src/completionsAtPosition.ts index 4beb54bb..c98ca4bd 100644 --- a/typescript/src/completionsAtPosition.ts +++ b/typescript/src/completionsAtPosition.ts @@ -1,6 +1,7 @@ import _ from 'lodash' import { compact } from '@zardoy/utils' import escapeStringRegexp from 'escape-string-regexp' +import isVueFileName from './utils/vue/isVueFileName' import inKeywordCompletions from './completions/inKeywordCompletions' import isInBannedPosition from './completions/isInBannedPosition' import { GetConfig } from './types' @@ -266,7 +267,6 @@ export const getCompletionsAtPosition = ( prior.entries = arrayMethods(prior.entries, position, sourceFile, c) ?? prior.entries prior.entries = jsdocDefault(prior.entries, position, sourceFile, languageService) ?? prior.entries - const isVueFileName = (fileName: string | undefined) => fileName && (fileName.endsWith('.vue.ts') || fileName.endsWith('.vue.js')) // #region Vue (Volar) specific const isVueFile = isVueFileName(fileName) if (isVueFile && exactNode) { diff --git a/typescript/src/utils.ts b/typescript/src/utils.ts index 1aea5e65..d84f09df 100644 --- a/typescript/src/utils.ts +++ b/typescript/src/utils.ts @@ -2,6 +2,7 @@ import { Except, SetOptional } from 'type-fest' import * as semver from 'semver' import type { MatchParentsType } from './utilTypes' +import isVueFileName from './utils/vue/isVueFileName' export function findChildContainingPosition(typescript: typeof ts, sourceFile: ts.SourceFile, position: number): ts.Node | undefined { function find(node: ts.Node): ts.Node | undefined { @@ -354,7 +355,7 @@ export const isValidInitializerForDestructure = (match: ts.Expression) => { return true } -export const isNameUniqueAtLocation = (name: string, location: ts.Node | undefined, typeChecker: ts.TypeChecker) => { +export const isNameUniqueAtLocation = (name: string, location: ts.Node, typeChecker: ts.TypeChecker) => { const checker = getFullTypeChecker(typeChecker) let hasCollision: boolean | undefined @@ -366,7 +367,6 @@ export const isNameUniqueAtLocation = (name: string, location: ts.Node | undefin childNode.forEachChild(checkCollision) } } - if (!location) return if (ts.isSourceFile(location)) { hasCollision = createUniqueName(name, location as any) !== name @@ -385,6 +385,8 @@ const getClosestParentScope = (node: ts.Node) => { } export const isNameUniqueAtNodeClosestScope = (name: string, node: ts.Node, typeChecker: ts.TypeChecker) => { const closestScope = getClosestParentScope(node) + if (!closestScope) return + return isNameUniqueAtLocation(name, closestScope, typeChecker) } @@ -423,6 +425,9 @@ const createUniqueName = (name: string, sourceFile: ts.SourceFile) => { return (parent as ts.ImportSpecifier).propertyName !== node case ts.SyntaxKind.PropertyAssignment: return (parent as ts.PropertyAssignment).name !== node + // Skip identifiers in vue template + case ts.SyntaxKind.ArrayLiteralExpression: + return !isVueFileName(sourceFile.fileName) default: return true } @@ -437,6 +442,7 @@ const createUniqueName = (name: string, sourceFile: ts.SourceFile) => { while (identifiers.includes(name)) { name = `_${name}` } + return name } diff --git a/typescript/src/utils/vue/isVueFileName.ts b/typescript/src/utils/vue/isVueFileName.ts new file mode 100644 index 00000000..42a1fd25 --- /dev/null +++ b/typescript/src/utils/vue/isVueFileName.ts @@ -0,0 +1 @@ +export default (fileName: string) => fileName.endsWith('.vue.ts') || fileName.endsWith('.vue.js') From 4ae4e527ba230797fc71de4f50c96c4843d53a8e Mon Sep 17 00:00:00 2001 From: Ilya Golovin Date: Fri, 16 Feb 2024 23:58:51 +0300 Subject: [PATCH 2/5] fix: handle call expression param case --- .../addDestructure/addSplittedDestructure.ts | 4 +-- .../test/codeActions/addDestruct.spec.ts | 27 ++++++++++++++++++- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/typescript/src/codeActions/custom/addDestructure/addSplittedDestructure.ts b/typescript/src/codeActions/custom/addDestructure/addSplittedDestructure.ts index 3021d68c..9df58687 100644 --- a/typescript/src/codeActions/custom/addDestructure/addSplittedDestructure.ts +++ b/typescript/src/codeActions/custom/addDestructure/addSplittedDestructure.ts @@ -22,8 +22,8 @@ export default (node: ts.Node, sourceFile: ts.SourceFile, formatOptions: ts.Form if ( ts.isElementAccessExpression(highlightedNode.parent) || - ts.isCallExpression(highlightedNode.parent.parent) || - ts.isTypeQueryNode(highlightedNode.parent) + ts.isTypeQueryNode(highlightedNode.parent) || + (ts.isCallExpression(highlightedNode.parent.parent) && highlightedNode.parent.parent.expression === highlightedNode.parent) ) return diff --git a/typescript/test/codeActions/addDestruct.spec.ts b/typescript/test/codeActions/addDestruct.spec.ts index 45433ebc..28c5d8a4 100644 --- a/typescript/test/codeActions/addDestruct.spec.ts +++ b/typescript/test/codeActions/addDestruct.spec.ts @@ -253,9 +253,34 @@ describe('Add destructure', () => { }) }) }) + describe('Should destruct param call expression param', () => { + test('Should skip if trying to destruct call expression', () => { + const initial = /* ts */ ` + const /*t*/newVariable/*t*/ = { test: 1} + + const obj = { + tag: foo.map(newVariable.test), + } + ` + const expected = /* ts */ ` + const { test } = { test: 1} + + const obj = { + tag: foo.map(test), + } + ` + + const { codeAction } = fourslashLikeTester(initial, undefined, { dedent: true }) + + codeAction(0, { + refactorName: 'Add Destruct', + newContent: expected, + }) + }) + }) describe('Skip cases', () => { - test('Should skip if trying to destruct call expression', () => { + test('Should skip if trying to destruct expression of call expression', () => { const initial = /* ts */ ` const /*t*/newVariable/*t*/ = foo From 59f7a6421eedf016f1c62c23efbbcf8f33cd1ee2 Mon Sep 17 00:00:00 2001 From: Ilya Golovin Date: Sun, 18 Feb 2024 17:31:03 +0300 Subject: [PATCH 3/5] feat: make addDestruct handle some vue reactivity case --- .../addDestructure/addSplittedDestructure.ts | 75 ++++++++-------- .../getDestructureReplaceInfo.ts | 47 ++++++++++ .../custom/addDestructure/vueSupportUtils.ts | 86 +++++++++++++++++++ typescript/src/codeActions/getCodeActions.ts | 8 +- 4 files changed, 174 insertions(+), 42 deletions(-) create mode 100644 typescript/src/codeActions/custom/addDestructure/getDestructureReplaceInfo.ts create mode 100644 typescript/src/codeActions/custom/addDestructure/vueSupportUtils.ts diff --git a/typescript/src/codeActions/custom/addDestructure/addSplittedDestructure.ts b/typescript/src/codeActions/custom/addDestructure/addSplittedDestructure.ts index 9df58687..b9e38991 100644 --- a/typescript/src/codeActions/custom/addDestructure/addSplittedDestructure.ts +++ b/typescript/src/codeActions/custom/addDestructure/addSplittedDestructure.ts @@ -1,5 +1,8 @@ -import { uniq } from 'rambda' -import { findChildContainingExactPosition, getChangesTracker, getPositionHighlights, isValidInitializerForDestructure, makeUniqueName } from '../../../utils' +import { uniqBy } from 'lodash' +import { getChangesTracker, getPositionHighlights, isValidInitializerForDestructure } from '../../../utils' +import isVueFileName from '../../../utils/vue/isVueFileName' +import { checkNeedToRefsWrap } from './vueSupportUtils' +import { getDestructureReplaceInfo } from './getDestructureReplaceInfo' export default (node: ts.Node, sourceFile: ts.SourceFile, formatOptions: ts.FormatCodeSettings | undefined, languageService: ts.LanguageService) => { const isValidInitializer = ts.isVariableDeclaration(node.parent) && node.parent.initializer && isValidInitializerForDestructure(node.parent.initializer) @@ -12,52 +15,32 @@ export default (node: ts.Node, sourceFile: ts.SourceFile, formatOptions: ts.Form if (!highlightPositions) return const tracker = getChangesTracker(formatOptions ?? {}) - const propertyNames: Array<{ initial: string; unique: string | undefined }> = [] - let nodeToReplaceWithBindingPattern: ts.Identifier | undefined + const res = getDestructureReplaceInfo(highlightPositions, node, sourceFile, languageService) - for (const pos of highlightPositions) { - const highlightedNode = findChildContainingExactPosition(sourceFile, pos) + if (!res) return - if (!highlightedNode) continue + const { propertiesToReplace, nodeToReplaceWithBindingPattern } = res - if ( - ts.isElementAccessExpression(highlightedNode.parent) || - ts.isTypeQueryNode(highlightedNode.parent) || - (ts.isCallExpression(highlightedNode.parent.parent) && highlightedNode.parent.parent.expression === highlightedNode.parent) - ) - return - - if (ts.isIdentifier(highlightedNode) && ts.isPropertyAccessExpression(highlightedNode.parent)) { - const accessorName = highlightedNode.parent.name.getText() + if (!nodeToReplaceWithBindingPattern || propertiesToReplace.length === 0) return - if (!accessorName) continue + const shouldHandleVueReactivityLose = + isVueFileName(sourceFile.fileName) && + ts.isVariableDeclaration(nodeToReplaceWithBindingPattern.parent) && + nodeToReplaceWithBindingPattern.parent.initializer && + checkNeedToRefsWrap(nodeToReplaceWithBindingPattern.parent.initializer) - const uniqueName = makeUniqueName(accessorName, node, languageService, sourceFile) + for (const { initial, range, unique } of propertiesToReplace) { + const uniqueNameIdentifier = ts.factory.createIdentifier(unique || initial) - propertyNames.push({ initial: accessorName, unique: uniqueName === accessorName ? undefined : uniqueName }) - const range = - ts.isPropertyAssignment(highlightedNode.parent.parent) && highlightedNode.parent.parent.name.getText() === accessorName - ? { - pos: highlightedNode.parent.parent.pos + highlightedNode.parent.parent.getLeadingTriviaWidth(), - end: highlightedNode.parent.parent.end, - } - : { pos, end: highlightedNode.parent.end } - - tracker.replaceRangeWithText(sourceFile, range, uniqueName) - continue - } - - if (ts.isIdentifier(highlightedNode) && (ts.isVariableDeclaration(highlightedNode.parent) || ts.isParameter(highlightedNode.parent))) { - // Already met a target node - abort as we encountered direct use of the potential destructured variable - if (nodeToReplaceWithBindingPattern) return - nodeToReplaceWithBindingPattern = highlightedNode + if (shouldHandleVueReactivityLose) { + const propertyAccessExpression = ts.factory.createPropertyAccessExpression(uniqueNameIdentifier, 'value') + tracker.replaceRange(sourceFile, range, propertyAccessExpression) continue } + tracker.replaceRange(sourceFile, range, uniqueNameIdentifier) } - if (!nodeToReplaceWithBindingPattern || propertyNames.length === 0) return - - const bindings = uniq(propertyNames).map(({ initial, unique }) => { + const bindings = uniqBy(propertiesToReplace, 'unique').map(({ initial, unique }) => { return ts.factory.createBindingElement(undefined, unique ? initial : undefined, unique ?? initial) }) @@ -73,6 +56,22 @@ export default (node: ts.Node, sourceFile: ts.SourceFile, formatOptions: ts.Form bindingPattern, ) + if (shouldHandleVueReactivityLose) { + // Wrap the `defineProps` with `toRefs` + const toRefs = ts.factory.createIdentifier('toRefs') + const unwrappedCall = nodeToReplaceWithBindingPattern.parent.initializer + const wrappedWithToRefsCall = ts.factory.createCallExpression(toRefs, undefined, [unwrappedCall]) + + tracker.replaceRange( + sourceFile, + { + pos: unwrappedCall.pos, + end: unwrappedCall.end, + }, + wrappedWithToRefsCall, + ) + } + const changes = tracker.getChanges() if (!changes) return undefined return { diff --git a/typescript/src/codeActions/custom/addDestructure/getDestructureReplaceInfo.ts b/typescript/src/codeActions/custom/addDestructure/getDestructureReplaceInfo.ts new file mode 100644 index 00000000..c180d8e9 --- /dev/null +++ b/typescript/src/codeActions/custom/addDestructure/getDestructureReplaceInfo.ts @@ -0,0 +1,47 @@ +import { findChildContainingExactPosition, makeUniqueName } from '../../../utils' + +export const getDestructureReplaceInfo = (highlightPositions: number[], node: ts.Node, sourceFile: ts.SourceFile, languageService: ts.LanguageService) => { + const propertiesToReplace: Array<{ initial: string; unique: string | undefined; range: { pos: number; end: number } }> = [] + let nodeToReplaceWithBindingPattern: ts.Identifier | undefined + + for (const pos of highlightPositions) { + const highlightedNode = findChildContainingExactPosition(sourceFile, pos) + + if (!highlightedNode) continue + + if ( + ts.isElementAccessExpression(highlightedNode.parent) || + ts.isTypeQueryNode(highlightedNode.parent) || + (ts.isCallExpression(highlightedNode.parent.parent) && highlightedNode.parent.parent.expression === highlightedNode.parent) + ) + return + + if (ts.isIdentifier(highlightedNode) && ts.isPropertyAccessExpression(highlightedNode.parent)) { + const accessorName = highlightedNode.parent.name.getText() + + if (!accessorName) continue + + const uniqueName = makeUniqueName(accessorName, node, languageService, sourceFile) + + const range = + ts.isPropertyAssignment(highlightedNode.parent.parent) && highlightedNode.parent.parent.name.getText() === accessorName + ? { + pos: highlightedNode.parent.parent.pos + highlightedNode.parent.parent.getLeadingTriviaWidth(), + end: highlightedNode.parent.parent.end, + } + : { pos, end: highlightedNode.parent.end } + + propertiesToReplace.push({ initial: accessorName, unique: uniqueName === accessorName ? undefined : uniqueName, range }) + + continue + } + + if (ts.isIdentifier(highlightedNode) && (ts.isVariableDeclaration(highlightedNode.parent) || ts.isParameter(highlightedNode.parent))) { + // Already met a target node - abort as we encountered direct use of the potential destructured variable + if (nodeToReplaceWithBindingPattern) return + nodeToReplaceWithBindingPattern = highlightedNode + continue + } + } + return { propertiesToReplace, nodeToReplaceWithBindingPattern } +} diff --git a/typescript/src/codeActions/custom/addDestructure/vueSupportUtils.ts b/typescript/src/codeActions/custom/addDestructure/vueSupportUtils.ts new file mode 100644 index 00000000..4b85501a --- /dev/null +++ b/typescript/src/codeActions/custom/addDestructure/vueSupportUtils.ts @@ -0,0 +1,86 @@ +import { findChildContainingExactPosition } from '../../../utils' + +export const checkAutoInsertDotValue = (sourceFile: ts.SourceFile, position: number, languageService: ts.LanguageService) => { + const node = findChildContainingExactPosition(sourceFile, position) + if (!node || isBlacklistNode(sourceFile, position)) return false + + const checker = languageService.getProgram()!.getTypeChecker() + const type = checker.getTypeAtLocation(node) + const props = type.getProperties() + + if (props.some(prop => prop.name === 'value')) return true + return false +} +/** + * Checks if the given expression needs to be wrapped with `toRefs` to preserve reactivity. + * @param expression The expression to check. + * @returns A boolean value indicating whether the expression needs to be wrapped. + */ +export const checkNeedToRefsWrap = (expression: ts.Expression) => { + const willLoseReactivityIfDestructFns = new Set(['defineProps', 'reactive']) + return Boolean(ts.isCallExpression(expression) && ts.isIdentifier(expression.expression) && willLoseReactivityIfDestructFns.has(expression.expression.text)) +} + +function isBlacklistNode(node: ts.Node, pos: number) { + if (ts.isVariableDeclaration(node) && pos >= node.name.getFullStart() && pos <= node.name.getEnd()) { + return true + } + if (ts.isFunctionDeclaration(node) && node.name && pos >= node.name.getFullStart() && pos <= node.name.getEnd()) { + return true + } + if (ts.isParameter(node) && pos >= node.name.getFullStart() && pos <= node.name.getEnd()) { + return true + } + if (ts.isPropertyAssignment(node) && pos >= node.name.getFullStart() && pos <= node.name.getEnd()) { + return true + } + if (ts.isShorthandPropertyAssignment(node)) { + return true + } + if (ts.isImportDeclaration(node)) { + return true + } + if (ts.isLiteralTypeNode(node)) { + return true + } + if (ts.isTypeReferenceNode(node)) { + return true + } + if (ts.isPropertyAccessExpression(node) && node.expression.end === pos && node.name.text === 'value') { + return true + } + if (ts.isCallExpression(node) && ts.isIdentifier(node.expression) && isWatchOrUseFunction(node.expression.text) && isTopLevelArgOrArrayTopLevelItem(node)) { + return true + } + + let _isBlacklistNode = false + node.forEachChild(node => { + if (_isBlacklistNode) return + if (pos >= node.getFullStart() && pos <= node.getEnd() && isBlacklistNode(node, pos)) { + _isBlacklistNode = true + } + }) + return _isBlacklistNode + + function isWatchOrUseFunction(fnName: string) { + return fnName === 'watch' || fnName === 'unref' || fnName === 'triggerRef' || fnName === 'isRef' || fnName.startsWith('use-') + } + function isTopLevelArgOrArrayTopLevelItem(node: ts.CallExpression) { + for (const arg of node.arguments) { + if (pos >= arg.getFullStart() && pos <= arg.getEnd()) { + if (ts.isIdentifier(arg)) { + return true + } + if (ts.isArrayLiteralExpression(arg)) { + for (const el of arg.elements) { + if (pos >= el.getFullStart() && pos <= el.getEnd()) { + return ts.isIdentifier(el) + } + } + } + return false + } + } + return false + } +} diff --git a/typescript/src/codeActions/getCodeActions.ts b/typescript/src/codeActions/getCodeActions.ts index c6ffe520..2e0eef1e 100644 --- a/typescript/src/codeActions/getCodeActions.ts +++ b/typescript/src/codeActions/getCodeActions.ts @@ -8,12 +8,12 @@ import changeStringReplaceToRegex from './custom/changeStringReplaceToRegex' import splitDeclarationAndInitialization from './custom/splitDeclarationAndInitialization' import declareMissingProperties from './extended/declareMissingProperties' import { renameParameterToNameFromType, renameAllParametersToNameFromType } from './custom/renameParameterToNameFromType' -import addDestructure_1 from './custom/addDestructure/addDestructure' -import fromDestructure_1 from './custom/fromDestructure/fromDestructure' +import addDestructure from './custom/addDestructure/addDestructure' +import fromDestructure from './custom/fromDestructure/fromDestructure' const codeActions: CodeAction[] = [ - addDestructure_1, - fromDestructure_1, + addDestructure, + fromDestructure, objectSwapKeysAndValues, changeStringReplaceToRegex, splitDeclarationAndInitialization, From ceffe315fd6372723e052c75755663e87b1eae0c Mon Sep 17 00:00:00 2001 From: Ilya Golovin Date: Sun, 18 Feb 2024 18:07:38 +0300 Subject: [PATCH 4/5] wip: vue support test --- .../test/codeActions/addDestruct.spec.ts | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/typescript/test/codeActions/addDestruct.spec.ts b/typescript/test/codeActions/addDestruct.spec.ts index 28c5d8a4..957d9c78 100644 --- a/typescript/test/codeActions/addDestruct.spec.ts +++ b/typescript/test/codeActions/addDestruct.spec.ts @@ -278,6 +278,52 @@ describe('Add destructure', () => { }) }) }) + describe.todo('Vue support', () => { + test('Should handle props reactivity lose', () => { + const initial = /* ts */ ` + const /*t*/props/*t*/ = defineProps({ + source: { + type: Object, + required: true, + }, + }); + ` + const expected = /* ts */ ` + const { source } = toRefs(defineProps({ + source: { + type: Object, + required: true, + }, + })); + ` + + const { codeAction } = fourslashLikeTester(initial, undefined, { dedent: true }) + + codeAction(0, { + refactorName: 'Add Destruct', + newContent: expected, + }) + }) + test('Should handle `reactive` object reactivity lose', () => { + const initial = /* ts */ ` + const /*t*/reactiveObject/*t*/ = reactive({ + source: 'str' + }); + ` + const expected = /* ts */ ` + const { source } = toRefs(reactive({ + source: 'str' + })); + ` + + const { codeAction } = fourslashLikeTester(initial, undefined, { dedent: true }) + + codeAction(0, { + refactorName: 'Add Destruct', + newContent: expected, + }) + }) + }) describe('Skip cases', () => { test('Should skip if trying to destruct expression of call expression', () => { From 013906caff51a553d4a4544e740a5ec5cf989506 Mon Sep 17 00:00:00 2001 From: Ilya Golovin Date: Sun, 18 Feb 2024 18:31:44 +0300 Subject: [PATCH 5/5] fix: type issue --- typescript/src/completionsAtPosition.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/typescript/src/completionsAtPosition.ts b/typescript/src/completionsAtPosition.ts index c98ca4bd..96a4eada 100644 --- a/typescript/src/completionsAtPosition.ts +++ b/typescript/src/completionsAtPosition.ts @@ -288,7 +288,11 @@ export const getCompletionsAtPosition = ( prior.entries = [] } if (c('cleanupVueComponentCompletions') === 'filter-non-vue') { - prior.entries = prior.entries.filter(entry => isVueFileName(entry.symbol?.declarations?.[0]?.getSourceFile().fileName)) + prior.entries = prior.entries.filter(entry => { + const fileName = entry.symbol?.declarations?.[0]?.getSourceFile().fileName + if (!fileName) return false + return isVueFileName(fileName) + }) } } }