Skip to content

Rewatch: restore backward compatibility for bsconfig.json#8368

Open
jfrolich wants to merge 11 commits intorescript-lang:masterfrom
jfrolich:rewatch-bsconfig-compat
Open

Rewatch: restore backward compatibility for bsconfig.json#8368
jfrolich wants to merge 11 commits intorescript-lang:masterfrom
jfrolich:rewatch-bsconfig-compat

Conversation

@jfrolich
Copy link
Copy Markdown
Collaborator

Summary

Restores backward compatibility that was removed in #8187 and #8196 so that existing projects using the legacy config name and field aliases continue to build under rewatch.

  • rewatch again falls back to bsconfig.json when rescript.json is absent (read_config, has_rescript_config).
  • The config parser accepts bs-dependencies, bs-dev-dependencies, and bsc-flags as aliases for dependencies, dev-dependencies, and compiler-flags (via #[serde(alias = ...)]).
  • When any of those legacy field names are present in the raw JSON, a deprecation warning is emitted pointing at the modern field name.
  • Reverts the cp bsconfig.json rescript.json workaround in rewatch/tests/suite.sh (no longer needed now that rewatch reads bsconfig.json directly).

Test plan

  • cargo test --lib config:: — 34 tests pass, including new alias/deprecation tests for bs-dependencies, bs-dev-dependencies, bsc-flags
  • Manual: place a package with a legacy bsconfig.json in node_modules and confirm rewatch resolves it and prints the deprecation warning
  • CI rewatch integration suite

🤖 Generated with Claude Code

Accept the legacy config file name and the old field aliases
(bs-dependencies, bs-dev-dependencies, bsc-flags) so existing
packages continue to build. Emit a deprecation warning on the
legacy field names pointing at the modern replacements.

Co-Authored-By: Claude <noreply@anthropic.com>
Comment thread rewatch/src/build/packages.rs
jfrolich and others added 4 commits April 19, 2026 14:27
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
…egacy-config test

read_package_name now also looks inside bsconfig.json if neither
package.json nor rescript.json carry the package name. The
warn_legacy_config build test is updated to assert the new behavior
(build succeeds + deprecation warning is printed).

Co-Authored-By: Claude <noreply@anthropic.com>
External packages can't fix these themselves, so the warning now
fires for any package (local or external). For external packages we
also look up an issue tracker URL from bugs/repository in package.json
and include it so users can report the issue upstream.

Co-Authored-By: Claude <noreply@anthropic.com>
@jfrolich jfrolich requested a review from cknitt April 19, 2026 12:46
@cknitt
Copy link
Copy Markdown
Member

cknitt commented Apr 19, 2026

@codex review

…efresh snapshots

Sorting the package iteration makes the warning output stable across
runs (AHashMap iteration order is otherwise nondeterministic). Snapshot
files are updated to reflect both the new deprecation messages
(bs-dependencies/bs-dev-dependencies/bsc-flags for @testrepo/deprecated-config,
and for the external rescript-nodejs + sury packages plus the
bsconfig.json filename deprecation on rescript-nodejs) and the new
sorted order.

Co-Authored-By: Claude <noreply@anthropic.com>
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3eac42748b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread rewatch/src/helpers.rs

fn has_rescript_config(path: &Path) -> bool {
path.join("rescript.json").exists()
path.join("rescript.json").exists() || path.join("bsconfig.json").exists()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Trigger full rebuilds for bsconfig.json edits

has_rescript_config now recognizes bsconfig.json, but watch mode still upgrades only rescript.json events to CompileType::Full (rewatch/src/watcher.rs:259-270) and ignores non-source .json changes otherwise. In a bsconfig-only project, editing config fields (dependencies, compiler flags, sources, etc.) will not trigger recompilation, so the running watcher can keep stale build state until manually restarted.

Useful? React with 👍 / 👎.

Comment thread rewatch/src/config.rs
Comment on lines +301 to 304
#[serde(alias = "bs-dependencies")]
pub dependencies: Option<Vec<String>>,
#[serde(rename = "dev-dependencies")]
#[serde(rename = "dev-dependencies", alias = "bs-dev-dependencies")]
pub dev_dependencies: Option<Vec<String>>,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Handle legacy alias + modern key without parse failure

Using serde aliases for legacy keys makes bs-dependencies/dependencies (and similar pairs) map to the same struct field, so configs that contain both names now deserialize as a duplicate field error. Those mixed configs were previously usable because the legacy key was treated as unknown, so this change can break migration scenarios; parsing should normalize/prefer the modern key while still emitting the deprecation warning.

Useful? React with 👍 / 👎.

jfrolich and others added 3 commits April 19, 2026 15:04
Previously watch mode only upgraded rescript.json changes to a full
rebuild. In a bsconfig-only project, editing config fields did not
trigger recompilation, so the running watcher kept stale build state
until restarted.

Co-Authored-By: Claude <noreply@anthropic.com>
Previously each deprecated field/filename printed its own paragraph,
which was noisy (especially for packages with multiple legacy fields,
like rescript-nodejs). The issue-tracker URL was also repeated per
warning. Collapsed into a single block per package:

  Package 'foo' uses deprecated config (support will be removed in a future version):
    - field 'bs-dependencies' — use 'dependencies' instead
    - filename 'bsconfig.json' — rename to 'rescript.json'
  Please report this to the package maintainer: https://…/issues

Snapshots and the warn_legacy_config build test are updated to match.

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
@cknitt
Copy link
Copy Markdown
Member

cknitt commented Apr 19, 2026

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f5a40743a1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread rewatch/src/config.rs
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Apr 19, 2026

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript@8368

@rescript/darwin-arm64

npm i https://pkg.pr.new/@rescript/darwin-arm64@8368

@rescript/darwin-x64

npm i https://pkg.pr.new/@rescript/darwin-x64@8368

@rescript/linux-arm64

npm i https://pkg.pr.new/@rescript/linux-arm64@8368

@rescript/linux-x64

npm i https://pkg.pr.new/@rescript/linux-x64@8368

@rescript/runtime

npm i https://pkg.pr.new/@rescript/runtime@8368

@rescript/win32-x64

npm i https://pkg.pr.new/@rescript/win32-x64@8368

commit: aaff088

@jfrolich jfrolich enabled auto-merge (squash) April 19, 2026 13:52
jfrolich and others added 2 commits April 19, 2026 16:04
package-specs.module now accepts the legacy 'cjs' value (alias for
'commonjs') and emits the standard per-package deprecation warning so
users know to migrate.

Co-Authored-By: Claude <noreply@anthropic.com>
package-specs.module now accepts the legacy 'es6' value (alias for
'esmodule') and emits the standard per-package deprecation warning.
The old test_package_specs_es6_deprecation (which asserted the value
was rejected) is replaced with test_es6_module_alias.

Co-Authored-By: Claude <noreply@anthropic.com>
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.

2 participants