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(compiler): ensure consistent treatment of comment nodes in default slots #9663

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

Conversation

jh-leong
Copy link
Member

@jh-leong jh-leong commented Nov 22, 2023

This pr resolves an inconsistency in the logic for handling default slots in packages/compiler-core/src/transforms/vSlot.ts. Previously, when <template v-slot> was used in any component slot, comment nodes were inadvertently filtered out. Conversely, in the absence of <template v-slot>, comment nodes were retained in the default slot. Playground.

The behavior is now unified to consistently retain comment nodes in the default slot, regardless of the presence of <template v-slot>.

Additionally, new unit tests have been added to cover scenarios involving comment nodes in default slots.

Relates to #9655

Copy link

github-actions bot commented Nov 22, 2023

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 99.1 kB 37.5 kB 33.8 kB
vue.global.prod.js 157 kB (-12 B) 57.3 kB (-5 B) 51 kB (-14 B)

Usages

Name Size Gzip Brotli
createApp 54.5 kB 21.1 kB 19.3 kB
createSSRApp 58.4 kB 22.8 kB 20.8 kB
defineCustomElement 59.1 kB 22.6 kB 20.6 kB
overall 68.1 kB 26.2 kB 23.8 kB

@baiwusanyu-c
Copy link
Member

/ecosystem-ci run

@vue-bot
Copy link
Contributor

vue-bot commented Nov 24, 2023

📝 Ran ecosystem CI: Open

suite result latest scheduled
language-tools failure failure
nuxt failure success
pinia success success
quasar success success
router success success
test-utils success success
vant success success
vite-plugin-vue success success
vitepress success success
vue-i18n success success
vue-macros success success
vuetify failure failure
vueuse success success
vue-simple-compiler success success

Copy link

netlify bot commented Dec 5, 2023

Deploy Preview for vue-next-template-explorer failed.

Name Link
🔨 Latest commit 8b8be0f
🔍 Latest deploy log https://app.netlify.com/sites/vue-next-template-explorer/deploys/656e8a3fd443c10009f3fe89

@haoqunjiang haoqunjiang added the 🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. label Mar 28, 2024
@skirtles-code
Copy link
Contributor

If I've understood correctly...

Currently, both of these examples would pass a comment VNode via the default slot:

<MyChild>
  <!-- foo -->
</MyChild>
<MyChild>
  <template v-slot>
    <!-- foo -->
  </template>
</MyChild>

Whereas this example would discard the comment and wouldn't pass a default slot function at all:

<MyChild>
  <!-- foo -->
  <template #bar></template>
</MyChild>

Adding other content, say a <div>, would lead to the default slot being passed, but only the <div> is included, the comment is still discarded:

<MyChild>
  <!-- foo -->
  <div />
  <template #bar></template>
</MyChild>

The proposal in this PR is to retain the comment in all cases, though the usual rules around stripping comments and compilerOptions.comments would still apply.

I don't know which is better.

On the one hand, the current behaviour seems inconsistent. As far as I'm aware, this magical stripping of comments is undocumented. There's an easy workaround, by using an explicit <template v-slot> for the default slot, but how would someone know that they need to do that?

On the other hand, it's easy to imagine someone putting in a comment before a <template #bar> just because they want to comment their code, not because they want to pass anything to the default slot. They wouldn't necessarily anticipate that adding a comment would lead to the inclusion of a default slot.

I think I prefer the proposal in this PR, but it isn't clear cut for me.

Copy link

pkg-pr-new bot commented Aug 27, 2024

commit: 4247ccf

@vue/compiler-core

pnpm add https://pkg.pr.new/@vue/compiler-core@9663

@vue/compiler-dom

pnpm add https://pkg.pr.new/@vue/compiler-dom@9663

@vue/compiler-sfc

pnpm add https://pkg.pr.new/@vue/compiler-sfc@9663

@vue/compiler-ssr

pnpm add https://pkg.pr.new/@vue/compiler-ssr@9663

@vue/reactivity

pnpm add https://pkg.pr.new/@vue/reactivity@9663

@vue/runtime-core

pnpm add https://pkg.pr.new/@vue/runtime-core@9663

@vue/runtime-dom

pnpm add https://pkg.pr.new/@vue/runtime-dom@9663

@vue/server-renderer

pnpm add https://pkg.pr.new/@vue/server-renderer@9663

@vue/shared

pnpm add https://pkg.pr.new/@vue/shared@9663

vue

pnpm add https://pkg.pr.new/vue@9663

@vue/compat

pnpm add https://pkg.pr.new/@vue/compat@9663

Open in Stackblitz

@edison1105
Copy link
Member

/ecosystem-ci run

@vue-bot
Copy link
Contributor

vue-bot commented Aug 27, 2024

📝 Ran ecosystem CI: Open

suite result latest scheduled
language-tools failure failure
nuxt failure failure
pinia success success
primevue success success
quasar success success
radix-vue success success
router success success
test-utils success success
vant success success
vite-plugin-vue success success
vitepress success success
vue-i18n success success
vue-macros success success
vuetify success success
vueuse success success
vue-simple-compiler success success

@edison1105 edison1105 added the ready for review This PR requires more reviews label Aug 27, 2024
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. ready for review This PR requires more reviews scope: compiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants