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(runtime-vapor): supports the use of slots with v-if #267

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
6 changes: 3 additions & 3 deletions packages/reactivity/src/effectScope.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,13 @@ export class EffectScope {
}
}

prevScope: EffectScope | undefined
prevScope: (EffectScope | undefined)[] = []
/**
* This should only be called on non-detached scopes
* @internal
*/
on() {
this.prevScope = activeEffectScope
this.prevScope.push(activeEffectScope)
activeEffectScope = this
}

Expand All @@ -77,7 +77,7 @@ export class EffectScope {
* @internal
*/
off() {
activeEffectScope = this.prevScope
activeEffectScope = this.prevScope.pop()
}

stop(fromParent?: boolean) {
Expand Down
43 changes: 43 additions & 0 deletions packages/runtime-vapor/__tests__/componentSlots.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import {
createComponent,
createForSlots,
createIf,
createSlot,
createVaporApp,
defineComponent,
Expand Down Expand Up @@ -672,5 +673,47 @@ describe('component: slots', () => {

expect(host.innerHTML).toBe('<p>fallback<!--slot--></p>')
})

test('should work with createIf', async () => {
const show = ref(true)
const spyConditionFn = vi.fn(() => show.value)
const t0 = template('<p>show</p>')
const t1 = template('<p>hide</p>')

const Child = defineComponent(() => {
const t0 = template('<p></p>')
const n1 = t0()
const n2 = createSlot('default')
insert(n2, n1 as ParentNode)
return n2
})

const { render, host } = define({
setup() {
return createComponent(Child, null, [
{
default: () =>
createIf(
spyConditionFn,
() => {
const n0 = t0()
return n0
},
() => {
const n1 = t1()
return n1
},
),
},
])
},
})
render()

expect(host.innerHTML).toBe('<p>show</p><!--if-->')
show.value = false
await nextTick()
expect(host.innerHTML).toBe('<p>hide</p><!--if-->')
})
})
})
5 changes: 0 additions & 5 deletions packages/runtime-vapor/__tests__/dom/prop.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,12 @@ import {
setCurrentInstance,
} from '../../src/component'
import { getMetadata, recordPropMetadata } from '../../src/componentMetadata'
import { getCurrentScope } from '@vue/reactivity'

let removeComponentInstance = NOOP
beforeEach(() => {
const instance = createComponentInstance((() => {}) as any, {}, null)
const reset = setCurrentInstance(instance)
const prev = getCurrentScope()
instance.scope.on()
removeComponentInstance = () => {
instance.scope.prevScope = prev
instance.scope.off()
reset()
removeComponentInstance = NOOP
}
Expand Down
2 changes: 2 additions & 0 deletions packages/runtime-vapor/src/component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,9 @@ export const getCurrentInstance: () => ComponentInternalInstance | null = () =>
export const setCurrentInstance = (instance: ComponentInternalInstance) => {
const prev = currentInstance
currentInstance = instance
instance.scope.on()
return () => {
instance.scope.off()
currentInstance = prev
}
}
Copy link
Member

@LittleSound LittleSound Aug 8, 2024

Choose a reason for hiding this comment

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

I am concerned this change may cause issues because:

  1. The setCurrentInstance will set instance.scope.on() internally and instance.scope.off() when calling reset, but as an internal Function, this hidden behavior is not very intuitive
  2. Not all situations that need to set currentInstance need to set instance.scope.on() at the same time, and this behavior is wrong in renderEffect.

related: #174

Expand Down
Loading