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.

Sign up for GitHub

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

feat(compiler-core): error if portal doesn't have target #384

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+29 −0
Open

feat(compiler-core): error if portal doesn't have target #384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
948353b
feat(compiler-core): error if portal doesn't have target
dsseng Oct 26, 2019
88895bc
Update packages/compiler-core/src/errors.ts
dsseng Oct 26, 2019
e7b553b
refactor(transformElement): use findProp
dsseng Oct 26, 2019
7bb7808
Merge branch 'master' into feat/validate-portal
yyx990803 Dec 9, 2019
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
20 changes: 20 additions & 0 deletions packages/compiler-core/__tests__/transforms/transformElement.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,26 @@ describe('compiler: element transform', () => {
assert(`Portal`)
})

test('should error on <Portal> element with no target', () => {
const onError = jest.fn()
parseWithElementTransform(`<portal foo="bar"><span /></portal>`, {
onError
})

expect(onError.mock.calls[0][0]).toMatchObject({
code: ErrorCodes.X_PORTAL_NO_TARGET
})
})

test('should not error on <Portal> element with bound target', () => {
const onError = jest.fn()
parseWithElementTransform(`<portal :target="bar"><span /></portal>`, {
onError
})

expect(onError).not.toHaveBeenCalled()
})

test('should handle <Suspense>', () => {
function assert(tag: string, content: string, hasFallback?: boolean) {
const { root, node } = parseWithElementTransform(
Expand Down
2 changes: 2 additions & 0 deletions packages/compiler-core/src/errors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ export const enum ErrorCodes {
X_V_MODEL_MALFORMED_EXPRESSION,
X_V_MODEL_ON_SCOPE_VARIABLE,
X_INVALID_EXPRESSION,
X_PORTAL_NO_TARGET,

// generic errors
X_PREFIX_ID_NOT_SUPPORTED,
Expand Down Expand Up @@ -175,6 +176,7 @@ export const errorMessages: { [code: number]: string } = {
[ErrorCodes.X_V_MODEL_MALFORMED_EXPRESSION]: `v-model value must be a valid JavaScript member expression.`,
[ErrorCodes.X_V_MODEL_ON_SCOPE_VARIABLE]: `v-model cannot be used on v-for or v-slot scope variables because they are not writable.`,
[ErrorCodes.X_INVALID_EXPRESSION]: `Invalid JavaScript expression.`,
[ErrorCodes.X_PORTAL_NO_TARGET]: `Portal requires a "target" prop.`,

// generic errors
[ErrorCodes.X_PREFIX_ID_NOT_SUPPORTED]: `"prefixIdentifiers" option is not supported in this build of compiler.`,
Expand Down
7 changes: 7 additions & 0 deletions packages/compiler-core/src/transforms/transformElement.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,13 @@ export const transformElement: NodeTransform = (node, context) => {
args.push(propsBuildResult.props)
}
}

if (node.tagType === ElementTypes.PORTAL && !findProp(node, 'target')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is an old PR, but given it's still open...

Should this maybe take account of v-bind="obj" too? The target could be passed in obj rather than as a separate prop.

Copy link
Member

@edison1105 edison1105 Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed with @skirtles-code. We also need to consider v-bind="obj", which cannot be done at compile phase. So we do it at runtime. see

core/packages/runtime-core/src/components/Teleport.ts

Lines 56 to 63 in 928af5f

const target = select(targetSelector)
if (__DEV__ && !target && !isTeleportDisabled(props)) {
warn(
`Failed to locate Teleport target with selector "${targetSelector}". ` +
`Note the target element must exist before the component is mounted - ` +
`i.e. the target cannot be rendered by the component itself, and ` +
`ideally should be outside of the entire Vue component tree.`,
)

I think this PR can be closed. @skirtles-code WDYT?

context.onError(
createCompilerError(ErrorCodes.X_PORTAL_NO_TARGET, node.loc)
)
}

// children
const hasChildren = node.children.length > 0
if (hasChildren) {
Expand Down