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 #1846: useStateful works with modelValue #1852

Merged
merged 2 commits into from
May 30, 2022

Conversation

Derranion
Copy link
Member

Description

Closes #1846

useStateful now works correctly with modelValue - previously modelValue updates were ignored completely.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Comment on lines 32 to 44
const modelValueRef = toRef(props, 'modelValue')
const statefulRef = toRef(props, 'stateful')
let unwatchModelValue: Function

const watchModelValue = () => {
unwatchModelValue = watch(modelValueRef, (modelValue) => {
valueState.value = modelValue
})
}

watch(statefulRef, (stateful: boolean) => {
stateful ? watchModelValue() : unwatchModelValue?.()
}, { immediate: true })
Copy link
Member

@RVitaly1978 RVitaly1978 May 26, 2022

Choose a reason for hiding this comment

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

Maybe it will work the same?

Suggested change
const modelValueRef = toRef(props, 'modelValue')
const statefulRef = toRef(props, 'stateful')
let unwatchModelValue: Function
const watchModelValue = () => {
unwatchModelValue = watch(modelValueRef, (modelValue) => {
valueState.value = modelValue
})
}
watch(statefulRef, (stateful: boolean) => {
stateful ? watchModelValue() : unwatchModelValue?.()
}, { immediate: true })
watch([() => props.stateful, () => props.modelValue], ([stateful, modelValue]) => {
if (stateful && modelValue !== undefined) {
valueState.value = modelValue
}
}, { immediate: true })

Copy link
Member Author

@Derranion Derranion May 26, 2022

Choose a reason for hiding this comment

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

This will never work because props.stateful and props.modelValue are not refs.

If we will make them refs, to make it work, there will be other issue - we will always have additional watcher for modelValue in every component when it is not needed.

Which is not optimal.

Copy link
Member

Choose a reason for hiding this comment

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

I updated my suggestion: replaced props with functions:
watch([() => props.stateful, () => props.modelValue], ([stateful, modelValue]) => {...

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok then

"there will be other issue - we will always have additional watcher for modelValue in every component when it is not needed.

Which is not optimal."

Copy link
Member

Choose a reason for hiding this comment

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

If there are no cases where stateful can change dynamically, we can do this:

if (props.stateful) {
  watch(() => props.modelValue, (modelValue) => {
    if (modelValue !== undefined) {
      valueState.value = modelValue
    }
  }, { immediate: true })
}

otherwise we can update your solution a bit:

let unwatchModelValue: Function

const watchModelValue = () => {
  unwatchModelValue = watch(() => props.modelValue, (modelValue) => {
    if (modelValue !== undefined) {
      valueState.value = modelValue
    }
  }, { immediate: true }) // to handle if passed defaultValue
}

watch(() => props.stateful, (stateful) => {
  stateful ? watchModelValue() : unwatchModelValue?.()
}, { immediate: true })

Copy link
Member Author

Choose a reason for hiding this comment

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

Stateful can be changed by the user any time.

undefined can be a valid modelValue in some cases.

Passed defaultValue is handled immediately here:
const valueState = ref(defaultValue === undefined ? props.modelValue : defaultValue) as Ref<T>

I've moved refs to functions though, indeed slightly less verbose but less explicit as well.

@Derranion Derranion merged commit 8ca0010 into epicmaxco:develop May 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stateful does not update from modelValue
4 participants