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

Third party Module type #40

Open
drselump14 opened this issue May 24, 2023 · 15 comments
Open

Third party Module type #40

drselump14 opened this issue May 24, 2023 · 15 comments

Comments

@drselump14
Copy link

Hi guys, thanks for the fantastic library. It rocks!

Recently I'm struggling with {:type_not_found, {PolymorphicEmbed, "t", "PolymorphicEmbed.t()"} error when using polymorphic_embed library.
The schema looks like this.

  typed_schema "todos" do
    field(:content, :string)
    field(:title, :string)

    polymorphic_embeds_one(:form,
      types: [
        product_form: Todos.ProductForm
      ],
      on_type_not_found: :raise,
      on_replace: :update
    )

Not sure If I can redefine the external module with Elixir, so does anyone know
what's the best practice to solve this issue?
Any help would be appreciated.

@dvic
Copy link
Collaborator

dvic commented May 24, 2023

Is it a compile error? Can you reproduce it in a livebook? Because I can't, here's what I tried

Untitled notebook

Mix.install([{:typed_ecto_schema, "~> 0.4.1"}, {:polymorphic_embed, "~> 3.0"}])
Resolving Hex dependencies...
Dependency resolution completed:
New:
  decimal 2.1.1
  ecto 3.10.1
  jason 1.4.0
  polymorphic_embed 3.0.5
  telemetry 1.2.1
  typed_ecto_schema 0.4.1
* Getting typed_ecto_schema (Hex package)
* Getting polymorphic_embed (Hex package)
* Getting ecto (Hex package)
* Getting jason (Hex package)
* Getting decimal (Hex package)
* Getting telemetry (Hex package)
You have added/upgraded packages you could sponsor, run `mix hex.sponsor` to learn more
==> decimal
Compiling 4 files (.ex)
Generated decimal app
===> Analyzing applications...
===> Compiling telemetry
==> jason
Compiling 10 files (.ex)
Generated jason app
==> ecto
Compiling 56 files (.ex)
Generated ecto app
==> typed_ecto_schema
Compiling 4 files (.ex)
Generated typed_ecto_schema app
==> polymorphic_embed
Compiling 2 files (.ex)
Generated polymorphic_embed app
:ok

Section

defmodule Foo do
  use TypedEctoSchema

  import PolymorphicEmbed

  typed_schema "todos" do
    field(:content, :string)
    field(:title, :string)

    polymorphic_embeds_one(:form,
      types: [
        product_form: Todos.ProductForm
      ],
      on_type_not_found: :raise,
      on_replace: :update
    )
  end
end
{:module, Foo, <<70, 79, 82, 49, 0, 0, 18, ...>>, :ok}

@drselump14
Copy link
Author

Hello @dvic, thanks for the response.
The errors come from Domo. It should be reproducible with the code below on livebook

Mix.install([
  {:domo, "~> 1.5"},
  {:typed_ecto_schema, "~> 0.4.1"}, 
  {:polymorphic_embed, "~> 3.0"}
])
defmodule Foo do
  use Domo, skip_defaults: true
  use TypedEctoSchema

  import PolymorphicEmbed

  typed_schema "todos" do
    field(:content, :string)
    field(:title, :string)

    polymorphic_embeds_one(:form,
      types: [
        product_form: Todos.ProductForm
      ],
      on_type_not_found: :raise,
      on_replace: :update
    )
  end
end

@bamorim
Copy link
Owner

bamorim commented May 26, 2023

Ok @drselump14, I experimented a little bit with this and there is a workaround, here is the analysis:

TypedEctoSchema + PolymorphicEmbed + Domo

Mix.install([
  {:domo, "~> 1.5"},
  {:typed_ecto_schema, "~> 0.4.1"},
  {:polymorphic_embed, "~> 3.0"}
])
:ok

Analysis and Workaround

defmodule Example1 do
  use TypedEctoSchema
  import PolymorphicEmbed

  typed_schema "todos" do
    field(:content, :string)
    field(:title, :string)

    polymorphic_embeds_one(:form,
      types: [
        product_form: Todos.ProductForm
      ],
      on_type_not_found: :raise,
      on_replace: :update
    )
  end
end
{:module, Example1, <<70, 79, 82, 49, 0, 0, 18, ...>>, :ok}

Ok, let's first understand what's happening on the example by checking the generated type t():

import IEx.Helpers
t(Example1.t())
@type t() :: %Example1{
        __meta__: Ecto.Schema.Metadata.t(),
        content: String.t() | nil,
        form: PolymorphicEmbed.t() | nil,
        id: integer() | nil,
        title: String.t() | nil
      }

This is happening because polymorhic_embeds_one(name, opts) is just a syntax sugar to field(name, PolymorphicEmbed, opts) and then, by default, TypedEctoSchema assumes there is a PolymorphicEmbed.t() which is not the case. So the code above is the same as this:

defmodule Example2 do
  use TypedEctoSchema

  typed_schema "todos" do
    field(:content, :string)
    field(:title, :string)

    field(:form, PolymorphicEmbed,
      types: [
        product_form: Todos.ProductForm
      ],
      on_type_not_found: :raise,
      on_replace: :update
    )
  end
end

t(Example2.t())
@type t() :: %Example2{
        __meta__: Ecto.Schema.Metadata.t(),
        content: String.t() | nil,
        form: PolymorphicEmbed.t() | nil,
        id: integer() | nil,
        title: String.t() | nil
      }

Of course, when creating TypedEctoSchema I followed some "sane defaults", but allowed for users to change it, so technically you could be able to do this using :: (it needs to match field, so that's why we can't use polymorphic_embeds_one with ::):

defmodule Example3 do
  use TypedEctoSchema

  typed_schema "todos" do
    field(:content, :string)
    field(:title, :string)

    field(:form, PolymorphicEmbed,
      types: [
        product_form: Todos.ProductForm
      ],
      on_type_not_found: :raise,
      on_replace: :update
    ) :: map() | nil
  end
end

t(Example3.t())
@type t() :: %Example3{
        __meta__: Ecto.Schema.Metadata.t(),
        content: String.t() | nil,
        form: map() | nil,
        id: integer() | nil,
        title: String.t() | nil
      }

With that in mind, a workaround would be:

defmodule Example4 do
  use TypedEctoSchema
  use Domo, skip_defaults: true

  typed_schema "todos" do
    field(:content, :string)
    field(:title, :string)

    field(:form, PolymorphicEmbed,
      types: [
        product_form: Todos.ProductForm
      ],
      on_type_not_found: :raise,
      on_replace: :update
    ) :: map() | nil
  end
end

t(Example4.t())
Domo is compiling type ensurer for 1 module (.ex)
@type t() :: %Example4{
        __meta__: Ecto.Schema.Metadata.t(),
        content: String.t() | nil,
        form: map() | nil,
        id: integer() | nil,
        title: String.t() | nil
      }

One important detail is that for the workaround to replicate polymoprhic_embeds_many you'll have to also add default: [] and use {:array, PolymorphicEmbed}, so would be something like:

defmodule Example5 do
  use TypedEctoSchema
  use Domo, skip_defaults: true

  typed_schema "todos" do
    field(:content, :string)
    field(:title, :string)

    field(:form, {:array, PolymorphicEmbed},
      types: [
        product_form: Todos.ProductForm
      ],
      on_type_not_found: :raise,
      on_replace: :update,
      default: []
    ) :: [map()]
  end
end

t(Example5.t())
Domo is compiling type ensurer for 1 module (.ex)
@type t() :: %Example5{
        __meta__: Ecto.Schema.Metadata.t(),
        content: String.t() | nil,
        form: [map()],
        id: integer() | nil,
        title: String.t() | nil
      }

Going forward

Of course there are a few things we could do to make this integration a little bit better. Here are some of my ideas (none decided yet):

  1. First-class integration with PolymorphicEmbed so it gets all the inner types and do a union.
  2. When evaluating :: recursively try to expand the left-side hand until we can match with an "Native Ecto" expression or until it cannot be expanded more.
  3. Verify if the "assumed" type exists and fall-back to term() if not.
  4. A "plugin" system for infering types for specific expressions (and then doing a job similar to 1)

The trade-offs for me are:

  • 1 would be the best experience, but if we plan to support all possible integration it can make TypedEctoSchema bloated and with the possibility of always breaking, so it might not be the best idea.
  • 2 not sure how that would perform or if there are other unintended consequences, but I think this can be a really nice idea.
  • 3 this would mean compile-time dependency to the used schemas and IIRC this was something @dvic did some work to remove and that would introduce it again. Also not sure how that would work with cyclic relationships.
  • 4 that would be the best in terms of future-proofing it, but can be much more complex to setup in a nice way (for example, should plugins be configured with a compile-time config or should we do on the use TypedEctoSchema, plugins: [...]?)

I like the PolymorphicEmbed library so I'd be ok doing 1 and then later we could try to extract into a Plugin system like 4. That being said, it might be a good idea to try to do 2 regardless of that.

@dvic
Copy link
Collaborator

dvic commented May 26, 2023

Nice write up @bamorim !

This is happening because polymorhic_embeds_one(name, opts) is just a syntax sugar to field(name, PolymorphicEmbed, opts) and then, by default, TypedEctoSchema assumes there is a PolymorphicEmbed.t() which is not the case.

can't we just expand a known list of macro's while evaluation the entries? (like polymorphic_embeds_one) Then we can just make sure to support polymorhic_embeds_one(name, opts) :: OtherType.t() to fix the incorrect typespec problem

@drselump14
Copy link
Author

Thanks @bamorim.
The 1st option would be a dream come true, although I also agree with @dvic comment for an easier approach

@kirillrogovoy
Copy link

Hey @drselump14,

Does mathieuprog/polymorphic_embed#96 fix this issue?

@bamorim
Copy link
Owner

bamorim commented May 27, 2024

It does make it slightly easier to work with, but is not the best "typing" you could get. Ideally we would look at all possible structs and compose the type that way, but as mentioned this should be maybe on a plug-in based way. This however is slight better. Thanks for doing that

@kirillrogovoy
Copy link

kirillrogovoy commented May 28, 2024

Agree that a composite type would be way better.

In the anticipation of the first-class typing I'm starting to slowly care less about type specs though. :D

Of course, production-ready typing for structs (and ubiquitous support in core libs and most dependencies) is probably not that close, but they make it clear that type specs will be phased out.

@bamorim
Copy link
Owner

bamorim commented May 31, 2024

Yeah, to be honest, me too. I don't want to make any big changes/moves as we wait for that to become a reality. Maybe at that time it can make sense for ecto to automatically generate the types itself or something like that.

Best we can do is wait. In the meantime we can maybe make this library help as much as possible.

@bvobart
Copy link

bvobart commented Sep 6, 2024

Hi, I'd also like to use TypedEctoSchema together with PolymorphicEmbed.

I would already be happy if I could simply write

defmodule Example1 do
  use TypedEctoSchema
  import PolymorphicEmbed

  typed_schema "todos" do
    field(:content, :string)
    field(:title, :string)

    polymorphic_embeds_one(:form,
      types: [
        product_form: Todos.ProductForm,
        order_form: Todos.OrderForm
      ],
      on_type_not_found: :raise,
      on_replace: :update
    ) :: Todos.ProductForm | Todos.OrderForm

    polymorphic_embeds_many(:children,
      types: [
        text_todo: Todos.TextTodo,
        visual_todo: Todos.VisualTodo
      ],
      on_type_not_found: :raise,
      on_replace: :delete
    ) :: [Todos.TextTodo | Todos.VisualTodo]
  end
end

Yes, it would definitely be better if the Polymorphic embedding type could be inferred, but this solution would at least allow me to continue using PolymorphicEmbed's DSL methods, instead of having to desugar them as @bamorim so expertly and extensively describes.

While I'm not familar with TypedEctoSchema's codebase – I did browse through the source code a bit, but I literally just discovered this library today – it seems that allowing polymorphic_embeds_one and polymorphic_embeds_many to receive type annotations in the same way this library allows for field and embeds_one etc, would be relatively simple to implement.

What do you think of this idea? And how simple/hard do you think it would be to implement that?


Note: @bamorim I was following your polymorphic_embeds_many example and I noticed I had to add array?: true to the list of field arguments to prevent a compilation error on typed_schema:

    field(:form, {:array, PolymorphicEmbed},
      types: [
        product_form: Todos.ProductForm
      ],
      on_type_not_found: :raise,
      on_replace: :update,
      default: [],
      array?: true
    ) :: [map()]

Thanks a lot for your extensive analysis and workarounds. Without it, I would have found this library unusable in combination with PolymorphicEmbed.

@a3kov
Copy link

a3kov commented Oct 10, 2024

I started to get warnings in VSCode about my custom Ecto type field not having type "t":
Unknown type: MyProject.EctoTypes.CustomField.t/0.ElixirLS Dialyzer

This workaround didn't help btw

field(:myfield, CustomField, null: false) :: term()

I can implement the t() type in this case, but it can't be done for third-party modules. There must be a way to avoid this situation ?

@bamorim
Copy link
Owner

bamorim commented Oct 13, 2024

That seems weird. Can you maybe give more details on that?

@a3kov
Copy link

a3kov commented Oct 13, 2024

Sorry, I've already removed typed_ecto_schema from the project

@vheathen
Copy link

vheathen commented Nov 7, 2024

Thank you for the great lib!

With more or less recent changes in polymorphic embeds it's not possible to easily use field/3 :: type form anymore (it would require adding additional fields like :array? and :default). So, maybe for now we can just extend the @schema_function_names and @embeds_function_names lists? Of course, plug-ins system would be the best solution, but it might take some time to go there.

@bamorim
Copy link
Owner

bamorim commented Nov 7, 2024

Honestly, we can probably just make it work seamlessly with some well known libraries for now. I think it will make things easier for people using. We might eventually want to switch to a plug-in based approach but if we can make it work with PolymorphicEmbed it might be good enough.

Unfortunately I don't have the time / mental capacity to do that right now but PRs are welcome.

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

No branches or pull requests

7 participants