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: forward compatibility with v9 on v-model props/events #6172

Merged
merged 10 commits into from
Nov 5, 2024

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Oct 28, 2024

☑️ Resolves

This PR adds forward compatibility with next (v9) on v-model props/events

  • modelValue prop fallbacks to an old prop
  • update:modelValue is emitted together with an old event
  • update:model-value is emitted as well to use the new Vue 3 recommended event style in template compatible with Vue 2

Implementation details:

  • Added useModelMigration to create migration proxy that:
    • Returns a new model prop if an old one is not defined
    • Triggers a new event together with an old one on set
    • Checks if either old or new prop is provided for required model
  • Use model proxy instead of old prop to get value
  • Assign to model instead of $emit an old event
  • Define new prop and event as well as define/update model option
  • Update docs:
    • Mark the old interfaces as deprecated in docs
    • Use v-model in docs examples

Screenshots

Docs

image

required: true

image

📃 Components

  • Check components with :checked + @update:checked:
    • NcActionCheckbox
    • NcActionRadio
    • NcCheckboxRadioSwitch
  • Text input fields with :value + @update:value:
    • NcActionTextEditable
    • NcInputField
    • NcPasswordField
    • NcRichContenteditable
    • NcTextArea
    • NcTextField
  • Selects (:value + @input):
    • NcSelect
    • NcSelectTags
    • NcSettingsSelectGroup
  • Pickers (:value + @input):
    • NcColorPicker
    • NcDateTimePicker
    • NcDateTimePickerNative
    • NcTimezonePicker
  • Special with :value + @update:value:
    • NcActionInput
  • Misc.:
    • NcSettingsInputText (:value + @update:value)

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

@ShGKme ShGKme added enhancement New feature or request 2. developing Work in progress labels Oct 28, 2024
@ShGKme ShGKme added this to the 8.20.0 milestone Oct 28, 2024
@ShGKme ShGKme requested a review from susnux October 28, 2024 22:01
@ShGKme ShGKme self-assigned this Oct 28, 2024
@ShGKme ShGKme changed the title feat: add compatibility with model prop and even with v9 feat: add compatibility with model prop and event with v9 Oct 29, 2024
@ShGKme ShGKme changed the title feat: add compatibility with model prop and event with v9 feat: add model prop/event compatibility with v9 Oct 30, 2024
* @return {import('vue').WritableComputedRef} - model proxy
*/
export function useModelMigration(oldModelName, oldModelEvent) {
const vm = getCurrentInstance()!.proxy
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure here, I had issues with this in past (accessing the proxy inside of the setup function).
Not sure if it was something in my code but for me I had to move the proxy access into the computed functions

Copy link
Contributor

Choose a reason for hiding this comment

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

We've been using similar approach for several major versions already in Talk for Vuex Store (useStore.js) in script setup components and composables, and so far didn't caught any errors with it.
Maybe some guard here with debug information would be safe (so if anyone encounters it, we can identify the reason)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@susnux This is not possible. This function returns either null or { proxy: instance }.

https://github.com/vuejs/vue/blob/main/src/v3/currentInstance.ts#L13

@ShGKme ShGKme marked this pull request as ready for review October 31, 2024 15:23
@ShGKme ShGKme changed the title feat: add model prop/event compatibility with v9 feat: forward compatibility with v9 on v-model props/events Oct 31, 2024
@ShGKme
Copy link
Contributor Author

ShGKme commented Oct 31, 2024

I understand this PR is quite huge. Maybe you can review at least some of them and check here?

  • Components with :checked + @update:checked:
    • NcActionCheckbox
    • NcActionRadio
    • NcCheckboxRadioSwitch (@Antreesy, tested with v-model and :checked.sync)
  • Text input fields with :value + @update:value:
    • NcActionTextEditable
    • NcInputField (@Antreesy, tested with v-model and :value.sync)
    • NcPasswordField (@Antreesy, tested with v-model and :value.sync)
    • NcRichContenteditable (@Antreesy, tested with v-model and :value.sync)
    • NcTextArea (@Antreesy, tested with v-model and :value.sync)
    • NcTextField (@Antreesy, tested with v-model and :value.sync)
  • Selects (:value + @input):
    • NcSelect (@Antreesy, tested with v-model and :model-value + @input)
    • NcSelectTags
    • NcSettingsSelectGroup
  • Pickers (:value + @input):
    • NcColorPicker (@Antreesy, tested with v-model and :value.sync)
    • NcDateTimePicker (@Antreesy, tested with v-model and :value.sync)
    • NcDateTimePickerNative
    • NcTimezonePicker
  • Special with :value + @update:value:
    • NcActionInput (@Antreesy, tested native picker with v-model and :value.sync)
  • Misc.:
    • NcSettingsInputText (:value + @update:value)

Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Tested used in Talk components, do not expect the rest to break anything

Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Tested some components, but reviewed code - looks sane!

@ShGKme ShGKme added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 5, 2024
@ShGKme
Copy link
Contributor Author

ShGKme commented Nov 5, 2024

🪄

@ShGKme ShGKme merged commit 9fd3d56 into master Nov 5, 2024
20 checks passed
@ShGKme ShGKme deleted the feat/v-model-compatibility branch November 5, 2024 15:35
Copy link
Contributor

@DorraJaouad DorraJaouad left a comment

Choose a reason for hiding this comment

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

Tested on components existing in Talk ✅ all good
Code-wise ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants