Skip to content
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

fix(reactivity-transform): don't overwrite scope and user imports #6840

Merged
merged 1 commit into from Nov 9, 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -199,6 +199,27 @@ exports[`object destructure w/ mid-path default values 1`] = `
"
`;

exports[`should not overwrite current scope 1`] = `
"
const fn = () => {
const $ = () => 'foo'
const $ref = () => 'bar'
const $$ = () => 'baz'
console.log($())
console.log($ref())
console.log($$())
}
"
`;

exports[`should not overwrite importing 1`] = `
"
import { $, $$ } from './foo'
$('foo')
$$('bar')
"
`;

exports[`should not rewrite scope variable 1`] = `
"import { ref as _ref } from 'vue'

Expand Down
Expand Up @@ -416,6 +416,35 @@ test('macro import alias and removal', () => {
assertCode(code)
})

// #6838
test('should not overwrite importing', () => {
const { code } = transform(
`
import { $, $$ } from './foo'
$('foo')
$$('bar')
`
)
assertCode(code)
})

// #6838
test('should not overwrite current scope', () => {
const { code } = transform(
`
const fn = () => {
const $ = () => 'foo'
const $ref = () => 'bar'
const $$ = () => 'baz'
console.log($())
console.log($ref())
console.log($$())
}
`
)
assertCode(code)
})

describe('errors', () => {
test('$ref w/ destructure', () => {
expect(() => transform(`let { a } = $ref(1)`)).toThrow(
Expand Down
100 changes: 71 additions & 29 deletions packages/reactivity-transform/src/reactivityTransform.ts
Expand Up @@ -8,7 +8,11 @@ import {
Program,
VariableDeclarator,
Expression,
VariableDeclaration
VariableDeclaration,
ImportDeclaration,
ImportSpecifier,
ImportDefaultSpecifier,
ImportNamespaceSpecifier
} from '@babel/types'
import MagicString, { SourceMap } from 'magic-string'
import { walk } from 'estree-walker'
Expand All @@ -25,6 +29,7 @@ import { hasOwn, isArray, isString, genPropsAccessExp } from '@vue/shared'

const CONVERT_SYMBOL = '$'
const ESCAPE_SYMBOL = '$$'
const IMPORT_SOURCE = 'vue/macros'
const shorthands = ['ref', 'computed', 'shallowRef', 'toRef', 'customRef']
const transformCheckRE = /[^\w]\$(?:\$|ref|computed|shallowRef)?\s*(\(|\<)/

Expand All @@ -48,6 +53,13 @@ export interface RefTransformResults {
importedHelpers: string[]
}

export interface ImportBinding {
local: string
imported: string
source: string
specifier: ImportSpecifier | ImportDefaultSpecifier | ImportNamespaceSpecifier
}

export function transform(
src: string,
{
Expand Down Expand Up @@ -115,39 +127,40 @@ export function transformAST(
// TODO remove when out of experimental
warnExperimental()

let convertSymbol = CONVERT_SYMBOL
let escapeSymbol = ESCAPE_SYMBOL
const userImports: Record<string, ImportBinding> = Object.create(null)
for (const node of ast.body) {
if (node.type !== 'ImportDeclaration') continue
walkImportDeclaration(node)
}

// macro import handling
for (const node of ast.body) {
if (
node.type === 'ImportDeclaration' &&
node.source.value === 'vue/macros'
) {
// remove macro imports
s.remove(node.start! + offset, node.end! + offset)
// check aliasing
for (const specifier of node.specifiers) {
if (specifier.type === 'ImportSpecifier') {
const imported = (specifier.imported as Identifier).name
const local = specifier.local.name
if (local !== imported) {
if (imported === ESCAPE_SYMBOL) {
escapeSymbol = local
} else if (imported === CONVERT_SYMBOL) {
convertSymbol = local
} else {
error(
`macro imports for ref-creating methods do not support aliasing.`,
specifier
)
}
}
}
let convertSymbol: string | undefined
let escapeSymbol: string | undefined
for (const { local, imported, source, specifier } of Object.values(
userImports
)) {
if (source === IMPORT_SOURCE) {
if (imported === ESCAPE_SYMBOL) {
escapeSymbol = local
} else if (imported === CONVERT_SYMBOL) {
convertSymbol = local
} else if (imported !== local) {
error(
`macro imports for ref-creating methods do not support aliasing.`,
specifier
)
}
}
}

// default symbol
if (!convertSymbol && !userImports[CONVERT_SYMBOL]) {
convertSymbol = CONVERT_SYMBOL
}
if (!escapeSymbol && !userImports[ESCAPE_SYMBOL]) {
escapeSymbol = ESCAPE_SYMBOL
}
Comment on lines +156 to +162
Copy link
Member Author

@sxzz sxzz Oct 9, 2022

Choose a reason for hiding this comment

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

Some opinions about the RFC: vuejs/rfcs#369 (comment)


const importedHelpers = new Set<string>()
const rootScope: Scope = {}
const scopeStack: Scope[] = [rootScope]
Expand All @@ -170,7 +183,32 @@ export function transformAST(
}
}

function walkImportDeclaration(node: ImportDeclaration) {
const source = node.source.value
if (source === IMPORT_SOURCE) {
s.remove(node.start! + offset, node.end! + offset)
}

for (const specifier of node.specifiers) {
const local = specifier.local.name
const imported =
(specifier.type === 'ImportSpecifier' &&
specifier.imported.type === 'Identifier' &&
specifier.imported.name) ||
'default'
userImports[local] = {
source,
local,
imported,
specifier
}
}
}

function isRefCreationCall(callee: string): string | false {
if (!convertSymbol || currentScope[convertSymbol] !== undefined) {
return false
}
if (callee === convertSymbol) {
return convertSymbol
}
Expand Down Expand Up @@ -628,7 +666,11 @@ export function transformAST(
)
}

if (callee === escapeSymbol) {
if (
escapeSymbol &&
currentScope[escapeSymbol] === undefined &&
callee === escapeSymbol
) {
s.remove(node.callee.start! + offset, node.callee.end! + offset)
escapeScope = node
}
Expand Down