bitwalker / timex_ecto

An adapter for using Timex DateTimes with Ecto
MIT License
162 stars 68 forks source link

Support for Ecto 3.0 #80

Closed nathany closed 5 years ago

nathany commented 5 years ago

I'm seeing a compile failure for timex_ecto 3.3.0 when putting {:ecto, "~> 3.0.0-rc.1"} in the Mix file.

arathunku commented 5 years ago

Details of the failure:

==> timex_ecto
Compiling 8 files (.ex)

== Compilation error in file lib/types/date.ex ==
** (CompileError) lib/types/date.ex:58: Ecto.Date.__struct__/0 is undefined, cannot expand struct Ecto.Date
    lib/types/date.ex:58: (module)
could not compile dependency :timex_ecto, "mix compile" failed. You can recompile this dependency with "mix deps.compile timex_ecto", update it with "mix deps.update timex_ecto" or clean it with "mix deps.clean timex_ecto"

Ecto.Date, Ecto.Time and Ecto.DateTime no longer exist. Instead developers should use Date, Time, DateTime and NaiveDateTime that ship as part of Elixir and are the preferred types since Ecto 2.1. Odds that you are already using the new types and not the deprecated ones.

https://github.com/elixir-ecto/ecto/blob/master/CHANGELOG.md

nathany commented 5 years ago

Maybe we can just remove this line? https://github.com/bitwalker/timex_ecto/blob/master/lib/types/date.ex#L58

and a few others

Though perhaps we should be using timex_ecto anymore?

kurtome commented 5 years ago

For reference, @josevalim mentioned the same thing when I erroneously opened this same bug against the ecto project: https://github.com/elixir-ecto/ecto/issues/2770

I thought we'd still need it to generate the models and migrations with the correct timestamp types, but I haven't had time to play with it.

nathany commented 5 years ago

I managed to remove timex_ecto from my project and just use timex + ecto.

kurtome commented 5 years ago

@nathany did you still use the Timex types for the timestamps()?

my models look like this right now:

defmodule Model.User do
  use Ecto.Schema
  use Timex.Ecto.Timestamps

  schema "users" do
    field(:email, :string, null: false)
    field(:login_time, Timex.Ecto.DateTime)
    ...

    timestamps()
  end
end
nathany commented 5 years ago

@kurtome Nope. I ended up using :utc_datetime instead of Timex.Ecto.DateTime.

kuroda commented 5 years ago

@kurtome

Taking advice in https://github.com/elixir-ecto/ecto/issues/2770#issuecomment-435140215, I created a custom Ecto type for my application:

defmodule MyApp.Ecto.DatetimeWithTimezone do
  @behaviour Ecto.Type
  def type, do: :datetime

  def cast(%DateTime{} = dt), do: {:ok, dt}
  def cast(_), do: :error

  def load(%NaiveDateTime{} = ndt), do: DateTime.from_naive(ndt, "Etc/UTC")
  def load(_), do: :error

  def dump(%DateTime{} = datetime) do
    case Timex.Timezone.convert(datetime, "Etc/UTC") do
      %DateTime{} = dt -> {:ok, dt}
      {:error, _} -> :error
    end
  end
  def dump(_), do: :error
end

Then, I made following modifications on my Model.User module:

defmodule Model.User do
  use Ecto.Schema
  # use Timex.Ecto.Timestamps           # <- Removed
  alias MyApp.Ecto.DatetimeWithTimezone # <- Added

  schema "users" do
    field(:email, :string, null: false)
    # field(:login_time, Timex.Ecto.Datetime) # <- Removed
    field(:login_time, DatetimeWithTimezone)  # <- Added
  ...

With these changes, my app works with the Ecto 3.0 just like before.

kurtome commented 5 years ago

For what it's worth, I think automatic conversion to UTC is in fact desirable, even if it means that dates aren't read from the database in the same timezone they were written in. From past projects I've worked on it's pretty common to write incoming dates to the database based on the user's timezone and then read them in UTC and convert back to the user's timezone for display purposes.

The important part is that that I'd expect the database layer (ecto_sql or timex_ecto in this case) to take care of is ensuring storage consistency in UTC. As long as the write/read object are equal using Timex.equal?/3, then I think automatic conversion is a-okay.

kuroda commented 5 years ago

@kurtome

I agree with you.

I submitted a question related to this issue to the Elixir forum:

https://elixirforum.com/t/how-to-upgrade-an-app-using-timex-ecto-to-phoenix-1-4-and-ecto-3-0/17676

Please make a comment on it to attract atttentions from community.

bitwalker commented 5 years ago

Closing as timex_ecto is no longer need in Ecto 3. I unfortunately do not have time to put together a guide, but hopefully the discussion above is enough for those that are searching for an upgrade path. If I manage to find more free time again soon I will put together a quick migration guide in the README.

angeleah commented 3 years ago

I think just adding "Timex_ectois no longer need in Ecto 3." in the README would save people newer to Elixir / Ecto a lot of time.