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

Changed custom Id logic to enable optional custom IDs #219

Merged
merged 2 commits into from
Feb 1, 2022
Merged

Changed custom Id logic to enable optional custom IDs #219

merged 2 commits into from
Feb 1, 2022

Conversation

mauriceackel
Copy link
Contributor

@mauriceackel mauriceackel commented Jan 29, 2022

🚨🚨 THIS IS A BREAKING CHANGE! 🚨🚨

Until now, when a user set customId: true, they had to always enter an ID.

This PR introduces new options for the customId property. Instead of setting a boolean value, the user selects either required or optional. If the value is required, a custom ID has to be entered (i.e., current behavior). If the value is optional, the user can leave the id field empty. In this case, an automatic ID will be used.

@fgatti675
Copy link
Member

Hi @mauriceackel
Thanks a lot for the PR, I think it is a good idea.
Since we are in the RC phase of version 1.0.0 I would rather not introduce breaking changes at this point.
I think we could easily solve this by still supporting the boolean type, which would work the same as before.
Think you can tweak it to keep the previous functionality?
Thanks!

@mauriceackel
Copy link
Contributor Author

mauriceackel commented Jan 31, 2022

UPDATE BELOW

Hey @fgatti675, sure I can adapt the PR.

There are two ways to go here:

  1. Make customId: boolean | 'optional' | EnumValue. This way, if customId == true, it would be the same behavior as currently (i.e. enforce entry of custom id)
  2. Make customId: boolean | 'required' | EnumValue. This way, if customId == true, the user may but does not have to enter a custom Id. If the value is 'required', the user is forced to enter a custom Id

Which way do you prefer? I guess the least breaking approach would be "1."

I just committed a change, it simply involves changing the type definition. With this change, approach "1." is implemented. Please tell me what you think about it!

@fgatti675 fgatti675 merged commit e178630 into firecmsco:master Feb 1, 2022
@fgatti675
Copy link
Member

Hi @mauriceackel
I have just released version @camberi/[email protected] with this change as a pre release, in case you want to test it or start using it before it is officially released ;)

@mauriceackel mauriceackel deleted the feature/cutom-id branch February 2, 2022 13:04
@mauriceackel
Copy link
Contributor Author

Hey @fgatti675,

I tested the branch and it looks good to me :) Thanks for the custom release! Looking forward to see this in production, there might need to be an update in the doc as well, what do you think?

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.

2 participants