Some suggestions

[Odonno]

Hi @benwinding

I just made some PRs in order to resolve some issues I had in the last weeks. I hope you'll like the improvements.

I have some other suggestions that can be discussed here if you want.

Suggestions

https://github.com/benwinding/react-admin-import-csv/blob/84fdf2fd800c1471be73d05246febca0bfeb6f44/src/import-csv-button.tsx#L16

I suppose that all console.log should be forbidden, right?

https://github.com/benwinding/react-admin-import-csv/blob/84fdf2fd800c1471be73d05246febca0bfeb6f44/src/import-csv-button.tsx#L18

I used to dispatch an action crudUpdateMany in order to save that in the redux store and then it will trigger an UPDATE_MANY event to the according data provider. You, otherwise, you use directly the data provider for each item. I am not sure what is the best use of this ATM. Otherwise, I can see there is 2 kind of operations:

• UPDATE if the primary key does not exist in the list
• CREATE if the primary key already exist

It can be a little tricky but I believe we should allow the developer to choose the action to execute in those cases, don't you think? Like adding a new prop.

https://github.com/benwinding/react-admin-import-csv/blob/84fdf2fd800c1471be73d05246febca0bfeb6f44/src/csv-extractor.ts#L14

Also, we manage papaparse props ourselves. That could be interesting to set default props and then let the developers override the props based on their own needs. Maybe by reusing the same standard of react-admin. They use an exporter prop, we could have an importer prop.

[benwinding]

Hey @Odonno,

I just want to first say thank you for sharing your code initially, it's been a big help in understanding react, redux and react-admin.

I suppose that all console.log should be forbidden, right?

Yes, or at least a configuration flag.

I used to dispatch an action crudUpdateMany in order to save that in the redux store and then it will trigger an UPDATE_MANY event to the according data provider. You, otherwise, you use directly the data provider for each item. I am not sure what is the best use of this ATM. Otherwise, I can see there is 2 kind of operations:

I'm not 100% sure, but according to the API, the UPDATE_MANY action updates a lot of ids with one row of common data. We need to update many ids with unique rows of data, so this probably isn't going to work. I may be mistaken though...

It can be a little tricky but I believe we should allow the developer to choose the action to execute in those cases, don't you think? Like adding a new prop.

Also, we manage papaparse props ourselves. That could be interesting to set default props and then let the developers override the props based on their own needs. Maybe by reusing the same standard of react-admin. They use an exporter prop, we could have an importer prop.

I like both of these ideas.

Cheers, Ben

[Odonno]

Hum, you are right, it seems that UPDATE_MANY is to update multiple objects using the same payload/properties. So, multiple UPDATE may be more suitable.

I also see you use hooks instead of the crudUpdate dispatched action. Seems to be a better way than I did. So, the last thing is to know to identify whether to create or to update. Not sure how to do that.

[benwinding]

I also see you use hooks instead of the crudUpdate dispatched action. Seems to be a better way than I did.

This was a huge learning process for me, I tried using effects and dispatches by kept getting "the invalid hook call warning". So after a lot of trial and error, this was the only thing that react allowed me to do.

... the last thing is to know to identify whether to create or to update. Not sure how to do that.

Maybe we could launch a Material-ui Dialog and ask the question to the user? (I think material-ui is included in react-admin anyway) Something like the following perhaps?

[Odonno]

That's a clever idea. react-admin already use Confirm modal to check for validation (like deletion). We should investigate on this to reuse their component.

Also, your idea gave some idea about new properties: overwrite and confirm. The confirm is a boolean to indicate if the modal should be opened. If confirm=false and overwrite is set, we should bypass the modal process to automatically execute the update/create.

That could be really good. The last thing that we should handle is when a user wants to import both new and updated data... Not sure how we can do that, because we cannot predict what user will put in the csv file.

[Odonno]

I suppose we can start like that with overwrite and confirm props. If someone has an idea in the future for improvements, we will check at that time.

What do you think?

[benwinding]

Hey @Odonno,

I've made some recent updates and included a demo here. Let me know what you think.

If you'd like to add some additional features or fix something, please have a look at the development notes at the bottom of the README, and submit a PR.

Cheers for your help, Ben