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

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

Closed
wants to merge 13 commits into from

Conversation

remotecom
Copy link

No description provided.

Alex Naser and others added 13 commits February 12, 2024 21:02
Update supported elixir and erlang versions
Remove support for MFA :with option
* Expand types aliases to avoid compile-time deps

* Add tests to cover :types defined via map

---------

Co-authored-by: Alex Naser <[email protected]>
chore: remove phoenix integration
Right now `cast_polymorphic_embed` for `polymorphic_embeds_many`, only creates new structs from the
module and ignores the data provided (normally the first argument of a
changeset/2 function). This leads to creating new ids for each and every
update when it's not required. This MR fixes this.
@remotecom remotecom closed this Apr 16, 2024
@mathieuprog
Copy link
Owner

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
Copy link
Owner

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

@Miradorn
Copy link

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
Copy link
Owner

mathieuprog commented May 27, 2024

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 #74 (comment) 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
Copy link

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
Copy link
Owner

Congrats to you then:) Enjoy your parental leave

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants