Skip to content

fix(compiler-core): KeepAlive should ignore comments #6026

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

amirkian007
Copy link

KeepAlive does not ignore comments and the compiler throws an error when a component and a comment are wrapped with KeepAlive, which is obviously a bug.

error reproduction :

sfc.vuejs.org

thanks.

@netlify
Copy link

netlify bot commented Jun 9, 2022

Deploy Preview for vuejs-coverage failed.

Name Link
🔨 Latest commit 77f5d44
🔍 Latest deploy log https://app.netlify.com/sites/vuejs-coverage/deploys/62a223332e6a04000831b817

@amirkian007
Copy link
Author

@tony19 hi can you re-review my new changes please ? the re-request review button not working...

Copy link
Contributor

@tony19 tony19 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the updates!

@iwusong
Copy link
Contributor

iwusong commented Jun 22, 2022

Comments cannot be ignored directly

@iwusong
Copy link
Contributor

iwusong commented Jun 22, 2022

https://staging-cn.vuejs.org/api/application.html#app-config-compileroptions

comments need final rendering on dom

@amirkian007
Copy link
Author

https://staging-cn.vuejs.org/api/application.html#app-config-compileroptions

comments need final rendering on dom

yeah comments should be rendered on the dom and the same problem exists for other built-in components too like Transition ,Suspense and etc . i dont know if it is a bug or not. but if it is, i think the fix for that should be submitted in another pr , this one is to just fix the warning issue with the compiler

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

4 participants