fix(js): read the format key only when it holds formatter options - #36
Closed
romankurakin wants to merge 1 commit into
Closed
romankurakin wants to merge 1 commit into
romankurakin wants to merge 1 commit into
Conversation
`Volt.Builder` reads `config :volt, :format` as the bundle format, where the value is an atom such as `:esm`. `Volt.JS.Format` and `Volt.JS.Helpers` read the same key as formatter options and expect a keyword list. A project that asks the bundler for ESM therefore cannot use `Volt.Formatter` or `mix volt.js.check`. `load_config/0` raises a `CaseClauseError`, and `discovery_config/1` passes the atom on to `Keyword.get/3`. Both now accept a keyword list and fall back otherwise. A configuration that worked before still works. A value that raised before now falls back to the JSON configuration.
There was a problem hiding this comment.
🟡 Changes recommended
The new Helpers.discover_format_files/0 == [] regression assertion is not hermetic and can start failing if matching JS/TS files are added under default discovery roots.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR resolves a configuration key collision where config :volt, :format can be either a bundler output format atom (e.g. :esm) or a keyword list of formatter/discovery options, causing formatter and JS-check tasks to crash when the bundler format is set.
Changes:
- Guard
Volt.JS.Format.load_config/0so it only treats:volt, :formatas formatter options when it’s a keyword list; otherwise it falls back to JSON config. - Guard
Volt.JS.Helpersdiscovery config so tool config is only used when it’s a keyword list, preventing crashes from non-keyword values. - Add regression tests covering the
:esmcollision case for both modules.
File summaries
| File | Description |
|---|---|
lib/volt/js/helpers.ex |
Ensures file discovery only consumes keyword-list tool config, ignoring atom bundle formats. |
lib/volt/js/format.ex |
Ensures formatter config only consumes keyword-list options and otherwise loads JSON config. |
test/volt/js/helpers_test.exs |
Adds regression coverage for discovery behavior when :format is an atom. |
test/volt/js/format_test.exs |
Adds regression coverage for load_config/0 fallback behavior when :format is an atom. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+56
to
+57
| assert Helpers.discover_format_files() == [] | ||
| assert Helpers.discover_files(tool: :lint) == [Path.join(tmp_dir, "lint/source.ts")] |
dannote
pushed a commit
that referenced
this pull request
Sep 14, 2026
Member
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #34.
Volt.Builderreadsconfig :volt, :formatas the bundle format, so its value is an atom such as:esm.Volt.JS.FormatandVolt.JS.Helpersread the same key as formatter options and expect a keyword list.A project that asks the bundler for ESM therefore cannot use
Volt.Formatterormix volt.js.check. Both fail before they look at a file.The two failures
Volt.JS.Format.load_config/0matches onlynilor a list, so the atom falls through thecase:Volt.JS.Helpers.discovery_config/1returns the atom unchanged, anddiscover_files/1then callsKeyword.get/3on it. This is the failure behindmix volt.js.check --type-aware:The change
Both functions now read the key only when it holds a keyword list, and fall back to the JSON configuration for anything else.
Nothing in the public API changes: no new function, no new configuration key. Configurations that work today keep working, and a value that used to raise now falls back instead.
Tests
Two regression tests, each next to the existing tests for the function it covers.
I also ran the branch against a Phoenix project that sets
format: :esm.mix volt.js.checkandmix volt.js.check --type-awareboth run, andVolt.Formatterworks as amix formatplugin.