Skip to content

chore(sync-service): upgrade Elixir to 1.20.0-rc.3#3992

Open
alco wants to merge 4 commits intomainfrom
alco/upgrade-elixir-otp
Open

chore(sync-service): upgrade Elixir to 1.20.0-rc.3#3992
alco wants to merge 4 commits intomainfrom
alco/upgrade-elixir-otp

Conversation

@alco
Copy link
Member

@alco alco commented Mar 10, 2026

Summary

  • Upgrade Elixir from 1.19.5 to 1.20.0-rc.3 (Erlang/OTP 28.4 unchanged)
  • Fix all Elixir 1.20 compatibility warnings in Electric source code:
    • Remove 7 unused require statements
    • Add pin operator (^) in 5 bitstring size patterns
    • Rename Changes.record() to Changes.row() (new built-in type conflict)
    • Remove unreachable :shutdown clause in pool.ex (improved type inference)
    • Use apply/3 in telemetry.ex to avoid compile-time conditional type warning
  • Bump postgrex 0.21.1 -> 0.22.0 (+ transitive deps db_connection, telemetry)

Known issue

Postgrex.Types.define generates code that Elixir 1.20's compiler flags as having unused functions. This is a postgrex compatibility issue — the 2 warnings come from generated code and cannot be suppressed from our side. Electric's own source is warning-free.

Test plan

  • mix compile passes (only 2 dep-generated warnings remain)
  • mix format --check-formatted passes
  • mix test passes (382 doctests, 8 properties, 1443 tests, 0 failures)
  • CI passes

🤖 Generated with Claude Code

@alco alco added the claude label Mar 10, 2026
@claude
Copy link

claude bot commented Mar 10, 2026

Claude Code Review

Summary

This PR upgrades Elixir from 1.19.5 to 1.20.0-rc.3, fixes all resulting compatibility warnings in Electric's source, and bumps postgrex to a git commit with the Elixir 1.20 compatibility fix. The changes are mechanical and correct.

What's Working Well

  • All previous review suggestions addressed: elixir-otp-upgrade-progress.md has been removed from the repo root (suggestion from iteration 1).
  • Postgrex approach is sound: Pinning to a specific git commit hash (e4f79427e99bb0dc86376a769fc966b98edb7dfc) is safer than pinning to a branch — the commit can't change unilaterally and the mix.lock captures it exactly. The TODO comment with a link to the upstream issue (elixir-ecto/postgrex#760) makes the temporary nature explicit and actionable.
  • apply/3 workaround is pragmatic: The change from Electric.telemetry_enabled?() to apply(Electric, :telemetry_enabled?, []) is also changed to use and instead of &&, which is the idiomatic Elixir boolean operator when short-circuit evaluation with non-boolean values isn't needed. Correct improvement.

Issues Found

Critical (Must Fix)

None.

Important (Should Fix)

Still upgrading to an RC, not a stable release

File: .tool-versions, packages/sync-service/Dockerfile

From iteration 1: the version being pinned is 1.20.0-rc.3. This is unchanged and is a deliberate choice. If 1.20.0 final is imminent, a follow-up issue to bump from RC to stable would ensure this doesn't get forgotten. The postgrex git dependency will also need to be resolved before/alongside that bump.

Suggestions (Nice to Have)

None remaining.

Issue Conformance

No linked issue (unchanged from iteration 1). The PR description clearly covers motivation and scope, so this is a minor point for a maintenance upgrade.

Previous Review Status

Issue Status
elixir-otp-upgrade-progress.md committed to repo root ✅ Fixed — file removed
RC version warning Still open (intentional)
No linked issue Still open (minor)

The two fixup commits addressed the postgrex warnings by switching from a not-yet-released ~> 0.22 hex version to the specific upstream git commit that contains the fix — a clean and well-documented approach.


Review iteration: 2 | 2026-03-11

@codecov
Copy link

codecov bot commented Mar 10, 2026

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 82.98%. Comparing base (69ba13c) to head (40ebf7e).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...lient/lib/electric/client/ecto_adapter/postgres.ex 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3992      +/-   ##
==========================================
- Coverage   85.73%   82.98%   -2.75%     
==========================================
  Files          51       65      +14     
  Lines        3343     3814     +471     
  Branches      612      609       -3     
==========================================
+ Hits         2866     3165     +299     
- Misses        475      647     +172     
  Partials        2        2              
Flag Coverage Δ
electric-telemetry 64.07% <ø> (?)
elixir 72.93% <0.00%> (-4.85%) ⬇️
elixir-client 77.18% <0.00%> (-0.60%) ⬇️
packages/experimental 87.73% <ø> (ø)
packages/react-hooks 86.48% <ø> (ø)
packages/start 82.83% <ø> (ø)
packages/typescript-client 93.93% <ø> (ø)
packages/y-electric 56.05% <ø> (ø)
typescript 88.74% <ø> (ø)
unit-tests 82.98% <0.00%> (-2.75%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@blacksmith-sh

This comment has been minimized.

@alco alco mentioned this pull request Mar 12, 2026
alco and others added 2 commits March 13, 2026 15:15
Upgrade Elixir from 1.19.5 to 1.20.0-rc.3 while keeping Erlang/OTP
at 28.4. Fix Elixir 1.20 compatibility warnings:

- Remove 7 unused `require` statements detected by stricter checks
- Add pin operator (`^`) in bitstring size patterns (5 occurrences)
- Rename `Changes.record()` type to `Changes.row()` to avoid
  overriding new built-in `record/0` type
- Remove unreachable `:shutdown` case clause in pool.ex (caught by
  improved type inference from preceding handle_info clause)
- Use `apply/3` in telemetry.ex to avoid type checker flagging
  compile-time conditional as always-false

Also bump postgrex 0.21.1 -> 0.22.0 (with db_connection and
telemetry transitive bumps).

Co-Authored-By: Claude Opus 4.6 <[email protected]>
Fix Elixir 1.20 warnings in code merged from main after rebase

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@alco alco force-pushed the alco/upgrade-elixir-otp branch from fe1f308 to 504092c Compare March 13, 2026 14:19
alco and others added 2 commits March 13, 2026 15:32
Remove leftover blank line after require Logger removal

Co-Authored-By: Claude Opus 4.6 <[email protected]>
Move require Logger inside conditional block in telemetry.ex and
add .tool-versions hash to CI cache keys to prevent stale caches
when upgrading Elixir/OTP.

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@blacksmith-sh

This comment has been minimized.

@alco alco marked this pull request as ready for review March 13, 2026 21:49
@alco alco self-assigned this Mar 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant