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

Make "default" optional in expanded form #487

Closed
MartkCz opened this issue Nov 11, 2021 · 5 comments · Fixed by #650
Closed

Make "default" optional in expanded form #487

MartkCz opened this issue Nov 11, 2021 · 5 comments · Fixed by #650
Assignees

Comments

@MartkCz
Copy link

MartkCz commented Nov 11, 2021

static values = {
  id: {
    type: String,
    required: true, // own implementation
  },
  method: String,
};

Type "string" must match the type of the default value. Given default value: "undefined" as "undefined"

@marcoroth
Copy link
Member

Hey @MartkCz, thanks for opening this issue!

Are you proposing to add the required option to the value definition, that the default value option should be optional if you use the expanded form or both?

I think it could make sense to apply both. The required option would default to false. If you explicitly set it to true and don't specify a value or if it doesn't have a default value it will raise with an exception (similar to what we have now).

I'm happy to implement this but I also want to give you the chance to try it yourself if you like.

@MartkCz
Copy link
Author

MartkCz commented Aug 8, 2022

The main reason was that default value is required in expanded form. However, it would be great if required option will be implemented in library. Unfortunately I don't have enough time right now to send PR :/

@marcoroth
Copy link
Member

No worries @MartkCz, I will take a look with what I can come up with. Thanks!

@marcoroth marcoroth self-assigned this Aug 11, 2022
@lucidstack
Copy link

Just adding a 👍 here, as the documentation doesn't make it explicit that the expanded form doesn't work without a default value. Spent an hour thinking I was going crazy until I found this!

@marcoroth
Copy link
Member

Thanks for the heads-up @lucidstack! 🙌🏼

I opened #650 to resolve this!

@dhh dhh closed this as completed in #650 Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants