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

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

Conversation

dsseng
Copy link
Contributor

@dsseng dsseng commented Oct 26, 2019

This will help people see their mistakes, erroring compilation of code failing in runtime. E.g. if user uses Vetur, they'll see mistakes in dev-time, not in runtime.

Examples of valid code:

<portal :target="a">
  <div>Portal content</div>
</portal>
<portal target="#a">
  <div>Portal content</div>
</portal>

Examples of erroring code:

<portal>
  I'm an error
</portal>
<portal a="b">
  I'm an error
</portal>

packages/compiler-core/src/errors.ts Outdated Show resolved Hide resolved
packages/compiler-core/src/transforms/transformElement.ts Outdated Show resolved Hide resolved
@LinusBorg LinusBorg added the 🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. label Feb 13, 2021
@LinusBorg
Copy link
Member

Even though an old PR, this still seems like a good addition.

Before getting back to this and solving merge conflicts though, we should fix some of the existing bugs first (see teleport labels)

@@ -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

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

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. 🛑 on hold This PR can't be merged until other work is finished scope: teleport
Projects
Status: Waiting
Development

Successfully merging this pull request may close these issues.

5 participants