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

A draft model instance with an empty ID returns an error when editing #223

Closed
Qsppl opened this issue Dec 7, 2023 · 6 comments
Closed
Labels
bug store Applies to the store feature

Comments

@Qsppl
Copy link
Contributor

Qsppl commented Dec 7, 2023

Example: https://codepen.io/qsppl/pen/WNPPdym?editors=1010

image

If the ID is defined, an error occurs when editing the draft
image

How to do it right? I tried using false instead of "" as the model id value - didn't work. Tried using -1 and returning an empty model, but that seems like a bad solution.

@smalluban smalluban added bug store Applies to the store feature labels Dec 7, 2023
@smalluban
Copy link
Contributor

smalluban commented Dec 7, 2023

It looks like you create not-covered edge cases ;) I made a fix, so just update to v8.2.8 and your case should work.

However, when using the draft mode, I recommend to not use the id setting. When the id is omitted, the model property itself can be set with a model instance or just an id. In your case you share the form with the presentation, which I also recommend to split. So then you have clean model form, which takes a model id, or not if it is a "create" mode:

{
 tag: 'a-comment-form',
 comment: store(Comment, { draft: true }),
 ...
};

Body of a comment component:

   <div>${comment.body}</div>
   ${editMode && html`<a-comment-form comment="${comment}"></a-comment-form>`}

And create form in another place (without setting the comment):

<p>Create comment</p>
<a-comment-form></a-comment-form>

@Qsppl
Copy link
Contributor Author

Qsppl commented Dec 7, 2023

Thank you. As for dividing the interface into presentation of models and forms for filling models with data, I wouldn’t want to do this. This reduces the accessibility of actions (in the case of separate edit pages or modal windows), or leads to duplication of markup and logic for "component" and "component-form" (in the case when for editing we replace the component with a form in the same place)

@Qsppl
Copy link
Contributor Author

Qsppl commented Dec 7, 2023

Example (changed*): https://codepen.io/qsppl/pen/WNPPdym?editors=1011

Clearing drafts no longer works.
It got id "2" and after that I can't set the component property to another draft.

image

@Qsppl
Copy link
Contributor Author

Qsppl commented Dec 7, 2023

If creation/editing and presentation really need to be separated into different components, then I’ll work like this. I don't understand very well how hybrids works and maybe I'm trying to do something impossible using one component to create/edit/present a model.

@smalluban
Copy link
Contributor

The problem is only with using the id setting. It then creates (and should) read-only property, so setting draft to null can't work - the property must always take the id from the property set by the id option.

You can change the draft instance by reassigning property, which is used for the id. However, it's impossible to clear draft with id pointing to undefined. You can't have cookie and eat cookie...

So.. I think using id option with draft should not be supported, as it creates that impossible cases. If you omit the id, and set the draft by its own property, everything will work.

Because of the above arguments, I recommend splitting the form from the comment component. But still you can use it inside of the comment, and also use it outside of that context.

Generally, I don't see it as redundant code, rather the opposite - splitting the logic into separate components is usually better.

@Qsppl
Copy link
Contributor Author

Qsppl commented Dec 7, 2023

Thanks, I'll do it like this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug store Applies to the store feature
Development

No branches or pull requests

2 participants