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(kselect): accept null in v-model [KHCP-13685] #2450

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

Conversation

portikM
Copy link
Member

@portikM portikM commented Oct 11, 2024

Summary

Jira: https://konghq.atlassian.net/browse/KHCP-13685

Typescript checks are getting tighter in v5.5.4, v-model now differentiates between undefined and null support. Since the backend APIs only recognize null as a value to unset an entry in the DB (particularly true for booleans) and not undefined, allow passing null as the ModelValue (treat passing a null the same as passing undefined)

@portikM portikM self-assigned this Oct 11, 2024
Copy link

netlify bot commented Oct 11, 2024

Deploy Preview for kongponents-sandbox ready!

Name Link
🔨 Latest commit ee3ba78
🔍 Latest deploy log https://app.netlify.com/sites/kongponents-sandbox/deploys/671285c7c7794d0008a61124
😎 Deploy Preview https://deploy-preview-2450--kongponents-sandbox.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Oct 11, 2024

Deploy Preview for kongponents ready!

Name Link
🔨 Latest commit ee3ba78
🔍 Latest deploy log https://app.netlify.com/sites/kongponents/deploys/671285c7a62048000882df8f
😎 Deploy Preview https://deploy-preview-2450--kongponents.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@portikM portikM changed the title fix(kselect, kcheckbox): accept null in v-model fix(kselect, kcheckbox, kinputswitch): accept null in v-model Oct 11, 2024
kaiarrowood
kaiarrowood previously approved these changes Oct 11, 2024
Copy link
Collaborator

@kaiarrowood kaiarrowood 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 Max!

@portikM portikM marked this pull request as ready for review October 11, 2024 21:42
Copy link
Member

@adamdehaven adamdehaven left a comment

Choose a reason for hiding this comment

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

issue: we should not be updating OSS component behavior based on Kong APIs that will change over time.

The host app should be responsible for handling sending the proper values to the API

src/components/KInputSwitch/KInputSwitch.vue Outdated Show resolved Hide resolved
@Justineo
Copy link
Contributor

issue: we should not be updating OSS component behavior based on Kong APIs that will change over time.

The host app should be responsible for handling sending the proper values to the API

Our internal requirements seem to reflect a common pattern, and there are considerations specifically from the frontend perspective. I suggest we focus the discussion on whether we want to allow null values for form components in general. A few questions to think about:

  • Should we consider foo a controlled prop if typeof props.foo !== 'undefined'?
  • If so, how should we represent the "controlled but unset" state for a <Select> component if null is not allowed? (We are currently using '', which feels a bit weird to me.)

@portikM
Copy link
Member Author

portikM commented Oct 15, 2024

  • Should we consider foo a controlled prop if typeof props.foo !== 'undefined'?

But that's basically what we are already doing and doesn't help us at all, plus it doesn't work for all Kongponents (KSelect)

The problem with this approach is it doesn't save us any work, we still have to do the conversion from away from null with the GET response and again convert back to null before we build the payload for PUT

@adamdehaven
Copy link
Member

The problem with this approach is it doesn't save us any work, we still have to do the conversion from away from null with the GET response and again convert back to null before we build the payload for PUT

This should be expected then.

We shouldn't be changing the component behavior based on the requirements of our APIs. Just as certain endpoints may require null, others may require undefined or have different logic, and that logic could change over time.

Mutating data to be acceptable by an external interface (e.g. Kong APIs) should be the responsibility of the host application.

@Justineo
Copy link
Contributor

Should we consider foo a controlled prop if typeof props.foo !== 'undefined'?

But that's basically what we are already doing and doesn't help us at all, plus it doesn't work for all Kongponents (KSelect)

I wasn't specifically referring to how accepting null makes it easier to connect to our backend API. Rather, if we're using undefined to differentiate between controlled and uncontrolled props, then null might be necessary to represent an explicitly empty value. And we should think about what null value represents for each component. Like what does null even mean for Checkbox or Switch?

We shouldn't be changing the component behavior based on the requirements of our APIs.

I agree. However, in this case, I was thinking less about it being a backend requirement and more about whether we should generally allow null values.

@portikM portikM changed the title fix(kselect, kcheckbox, kinputswitch): accept null in v-model fix(kselect, kmultiselect): accept null in v-model Oct 16, 2024
@portikM
Copy link
Member Author

portikM commented Oct 16, 2024

Okay, so I've updated this PR to support null in KSelect only. I've tested both components in sandbox and they looked good.

cc @kaiarrowood @Justineo @adamdehaven

@portikM portikM changed the title fix(kselect, kmultiselect): accept null in v-model fix(kselect): accept null in v-model [KHCP-13685] Oct 17, 2024
@portikM portikM enabled auto-merge (squash) October 17, 2024 17:46
@kongponents-bot
Copy link
Collaborator

Preview package from this PR in consuming application

In consuming application project install preview version of kongponents generated by this PR:

@kong/kongponents@pr-2450

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.

5 participants