search

mathieuprog / polymorphic_embed

Polymorphic embeds in Ecto
Apache License 2.0
325 stars 63 forks source link

Support updating (while retaining ids) on `cast_polymorphic_embed` for `polymorphic_embeds_many`. #95

Closed remotecom closed 5 months ago

mathieuprog commented 3 months ago

Thank you for the work, few questions:

@naserca Why did you remove support for MFA :with? If I'm not wrong this is supported by the Ecto embeds_ functions so therefore polymorphic embed supports this as well.

@Miradorn Why did you remove the dependencies on phoenix html/phoenix ecto/phoenix live view in the mix file?

Anyway the changes have been added in the lib. Thank you

mathieuprog commented 3 months ago

@naserca Okay found the deprecation notice of using MFA in Ecto

Miradorn commented 3 months ago

Hi @mathieuprog , thanks for following up and taking some time to work on this!

IIRC I removed the phoenix_html related code due to a problem when we updated phoenix_html (or some of the other phoenix libraries). The new version had breaking api changes which broke compilation of the related modules here. Since we weren't using the html part of this library anyway internally I decided it was easier (for us :D) to remove the dependency completely instead of trying to make it work either with both versions or break support of the old one.

I think it might be worthwhile to extract the phoenix_html specific code into its own package (polymophic_embed_html_helpers? :D) to avoid this happening again in the future, ignoring the overhead of that approach 🤐 , since it might be a common use-case to use the "core" functionality without the html helpers.

In the end it's obviously your call! Thanks again for the great library and the work ❤️

mathieuprog commented 3 months ago

Thank you for the clarifications. Taking note of the suggestion to isolate the helpers, feel free to contribute it or I'll address this later.

In the meanwhile all of the changes from your branch have been added.

I also noticed your comment here https://github.com/mathieuprog/polymorphic_embed/issues/74#issuecomment-1439622961 but we would need a failing test. Again welcome to add it, or I'll address this later as well. If the comment is related to the issue, this would allow us to understand the limitation we have with custom Ecto Type and either see if anything can be done on Ecto's side, or at the minimum document this limitation.

Miradorn commented 3 months ago

I would love to spent some time, but I'm going on parental leave next week for 4 months, so I'm currently pretty swamped finishing up work and preparing handover. Once I'm getting back and something needs work/help I'll be glad to jump onto it!

mathieuprog commented 3 months ago

Congrats to you then:) Enjoy your parental leave