hybridsjs / hybrids

Extraordinary JavaScript UI framework with unique declarative and functional architecture
https://hybrids.js.org
MIT License
3.03k stars 85 forks source link

An error occurs when saving a Draft #220

Closed Qsppl closed 9 months ago

Qsppl commented 9 months ago

Example:

https://codepen.io/qsppl/pen/mdvKrWP?editors=1011

We create several forms with the model in “Draft” mode. image

Let's try to save them. image

We see a bug:

Local and storage data must have the same id: '3c981b3b-f454-4402-a51f-92569fbe36ac', '7'

I tried to write a solution without linking Draft to "id", but it didn't work:

https://codepen.io/qsppl/pen/wvNEjBw?editors=1011

Qsppl commented 9 months ago

I tried to solve the problem through a single model descriptor that does not have an Id and is always in Draft mode. It didn't work out either. All components without a defined model receive the same model after the first model is created in one of the components:

https://codepen.io/qsppl/pen/NWoLMYR?editors=1011

smalluban commented 9 months ago

https://github.com/hybridsjs/hybrids/blob/main/src/store.js#L1528

The draft model when used without an id falls back to the undefined id - so, indeed, if you have multiple components using a draft mode of the same original model, and not set id, they will all have the same "draft" instance. That behavior is very rare to have multiple "create" forms of the same model at once on the screen. Otherwise, the store would have to create id on the fly - but how it could then say, that it is not the normal id of the original model - so it would be very complicated to make it work like this.

I think what you can do, is to add the id setting to your draftComment - so then if it is "edit" mode - the component has set commentId property.

{  
  commentId: '',
  draftComment: store(Comment, { draft: true, id: 'commentId' }),
  comment: store(Comment, { id: 'commentId' }),
}

I'll try to make a working example tomorrow.

Qsppl commented 9 months ago

This is the case from the editable table

image

smalluban commented 9 months ago

I've started building an example, and I think I finally understand your case. If I am correct, you have some sort of rows, and you can add a comment to each, but it is a "create" form - and in your case, a user can open multiple forms at once.

Unfortunately, the current implementation of the draft model fails in that case, as I wrote earlier, you can't have multiple instances of the draft without passing id.

I'll try to rethink what can be done for you. In the meantime, I hope that you have some workaround of the issue, and it does not block you.

EDIT: The easiest workaround could be to not use draft when adding a comment, just use store.set() directly with new data when the form is submitted. I know that then you miss validation from the store (if you use it).

Qsppl commented 9 months ago

I solved the problem. I'm showing only one on-demand creation form.

image

image

But it is very inconvenient to control the components so that only one form is open at a time.

I also had to create two components:

Instead of one:

Duplicate html and css markup. This becomes a problem when there are more than 10 models in the table. Each model requires two identical components instead of one.

But this solution supports Store validation.

smalluban commented 9 months ago

Good to know that you have temporary solution. I'll try to rethink it as soon as possible, and back with more general way to handle that case.

smalluban commented 9 months ago

After all, I figured out, that the store can create a local temporary ID for the draft model, as it is very unlikely that it might hit an existing ID of the storage (for UUID v4 the collision is negligible).

If it is possible, please test it out before merging and release, using codesandbox build:

"dependencies": {
    "hybrids": "https://pkg.csb.dev/hybridsjs/hybrids/commit/e530588f/hybrids"
  },

You can use it directly in your code or using codesandbox (link in the PR mentioned above).

smalluban commented 9 months ago

Please try v8.2.7 where you can safely use multiple instances of the component that uses store draft without id. It should work now as you expected.

Qsppl commented 9 months ago

Please try v8.2.7 where you can safely use multiple instances of the component that uses store draft without id. It should work now as you expected.

Thank you very much, I didn’t expect you to add this functionality so quickly. I'll try to use this in the next similar case. The task that I solved was already in production.