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

Update to LiveView 1.0 🎉 #826

Merged
merged 4 commits into from
Dec 11, 2024
Merged

Conversation

solnic
Copy link
Collaborator

@solnic solnic commented Dec 5, 2024

Closes #825

@solnic solnic linked an issue Dec 5, 2024 that may be closed by this pull request
@solnic solnic marked this pull request as ready for review December 5, 2024 13:14
@solnic
Copy link
Collaborator Author

solnic commented Dec 5, 2024

@whatyouhide what's the reasoning behind having mix.lock files in git?

Copy link
Collaborator

@whatyouhide whatyouhide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general is so that you get reproducible mix deps.get every time instead of making it depend on what versions are up on Hex. For libraries like this one, dependants don't read mix.lock, it's only for local use when developing the library. Does it make sense?

mix.exs Outdated Show resolved Hide resolved
test_integrations/phoenix_app/mix.exs Outdated Show resolved Hide resolved
@solnic
Copy link
Collaborator Author

solnic commented Dec 6, 2024

In general is so that you get reproducible mix deps.get every time instead of making it depend on what versions are up on Hex. For libraries like this one, dependants don't read mix.lock, it's only for local use when developing the library. Does it make sense?

@whatyouhide yeah it does but the downside is that we're locking ourselves to specific versions, and then people release new versions of stuff we depend on and we're missing running our test suite against these updated versions. My approach for years was to always let the CI install whatever is the latest. This also helps catch installation issues early on. There are of course special cases like explicitly testing against a very specific set of versions for some crucial deps. In our case that would be of course Phoenix and LiveView.

I'm not saying we should remove the locks though, just raising this as a concern. We could solve this in various ways - like having dependabot configured for our needs, or a separate CI run that does mix deps.update --all.

@whatyouhide
Copy link
Collaborator

Yeah we shouldn't remove the locks. Dependabot is probably what we want here.

@marpo60
Copy link

marpo60 commented Dec 6, 2024

👋 👋 👋

Not sure if it was on your radar but we built blend to solve this issue. You can test an hex package against different version of its dependencies.

This is an example of surface-ui testing against phoenix_live_view 0.20.x and 0.19.x

@phinnaeus
Copy link

@solnic it's best practice not just with elixir but in pretty much every language that supports it to commit your lockfiles. that's the whole point of the lockfile concept, for reproducibility. note this is not the same as vendoring your dependencies.

Copy link
Collaborator

@whatyouhide whatyouhide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also rebase on master please? 🙃

mix.exs Outdated Show resolved Hide resolved
@solnic solnic force-pushed the solnic/825-liveview-has-reached-100 branch from 5a46d23 to 5169ff4 Compare December 9, 2024 07:56
@solnic
Copy link
Collaborator Author

solnic commented Dec 9, 2024

@whatyouhide I'm not sure how to make this work with 1.13 and locks checked in. Any ideas? Do we need a separate lock for 1.13? When installing deps on CI we can't have locks that point to LV 1.0 because that will make 1.13 install it and crash. Having "~> 0.20 or ~> 1.0" doesn't make 1.13 install older LV (unsurprisingly).

...unless I'm missing something obvious?

@whatyouhide
Copy link
Collaborator

For now patch CI so that you run mix deps.unlock phoenix_live_view if the Elixir version is older. This is temporary anyway since it'll go away once we drop support for older Elixir versions.

@solnic
Copy link
Collaborator Author

solnic commented Dec 9, 2024

@solnic it's best practice not just with elixir but in pretty much every language that supports it to commit your lockfiles. that's the whole point of the lockfile concept, for reproducibility. note this is not the same as vendoring your dependencies.

In my experience, it is FAR simpler to not have them checked in for OSS projects when you care about staying on top of the deps (as in, test against latest versions). Locks checked in make perfect sense for apps, but not for OSS libs. The whole point of running CI for OSS libs is to ensure that things don't break and your lib can be installed, it is so easy to get behind by an accident. I understand that you can have locks checked in and use other tools to inform you about outdated deps, but from a pragmatic/time-saver POV it is just extra work. I've found it to be just simpler and less time consuming to not have locks in OSS lib repos.

This is my personal opinion and experience - our situation here is different :)

@whatyouhide
Copy link
Collaborator

Peter, I have never experienced issues with checked-in lockfiles or had my time consumed 🙃 We have a path forward here, so let's just take that and merge this PR 💯

@phinnaeus
Copy link

@whatyouhide I'm not trying to argue or hold anything up, just for my own curiosity -- what's the problem with using >= 20.0? It seems like it would allow consumers to use either 1.0 or 0.20 unlike ~> 0.20 is equivalent to >= 0.20 and < 0.21.

@solnic solnic force-pushed the solnic/825-liveview-has-reached-100 branch from 6035a0f to 768ae60 Compare December 9, 2024 10:19
@Munksgaard
Copy link
Contributor

@phinnaeus: I'm pretty sure Mix supports or clauses in the version requirements. Perhaps "~> 0.20 or ~> 1.0" is more reasonable?

@solnic
Copy link
Collaborator Author

solnic commented Dec 9, 2024

With "or" it still installs the latest version because that's what we have in the lock, unless I'm doing something wrong here :(

@solnic
Copy link
Collaborator Author

solnic commented Dec 9, 2024

@whatyouhide I'm not trying to argue or hold anything up, just for my own curiosity -- what's the problem with using >= 20.0?

Open-ended dep requirements are problematic because 2.0.0 may no longer be compatible, we could have ">= 1.0 and < 2.0" though.

@phinnaeus
Copy link

Gotcha, yeah I understand now. To give context to my earlier replies, I wasn't considering the requirement to test against both 0.x and 1.x versions of Live View.

@solnic solnic force-pushed the solnic/825-liveview-has-reached-100 branch 2 times, most recently from 5250468 to 88cdd53 Compare December 10, 2024 08:54
@solnic solnic force-pushed the solnic/825-liveview-has-reached-100 branch from 88cdd53 to dae1ef2 Compare December 10, 2024 08:57
@solnic solnic requested a review from whatyouhide December 10, 2024 09:00
@solnic solnic requested a review from phinnaeus December 10, 2024 09:00
@solnic
Copy link
Collaborator Author

solnic commented Dec 10, 2024

@whatyouhide @phinnaeus looks like I've made it 🙃 the trick was to skip doing deps.get in test/integrations since we don't even run these tests under 1.13, and also skip doing --check-locked under 1.13

@whatyouhide whatyouhide merged commit ce8654b into master Dec 11, 2024
4 checks passed
@whatyouhide whatyouhide deleted the solnic/825-liveview-has-reached-100 branch December 11, 2024 07:32
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.

LiveView has reached 1.0.0
5 participants