Skip to content

chore: migrate xml-tools - #620

Merged
rolanbadrislamov merged 9 commits into
mainfrom
migrate/xml-tools
Sep 21, 2026
Merged

chore: migrate xml-tools#620
rolanbadrislamov merged 9 commits into
mainfrom
migrate/xml-tools

Conversation

@rolanbadrislamov

@rolanbadrislamov rolanbadrislamov commented Sep 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Migrates SAP/xml-tools into projects/xml-tools/

What changed:

  • 10 packages moved into projects/xml-tools/packages/ (9 published @xml-tools/* libraries + the private xml-toolkit VS Code extension)
  • Wired into the monorepo toolchain: workspace:* intra-project deps, per-package tsconfig/nyc config, root eslint overrides
  • Added transitive deps that yarn's flat node_modules had masked (chevrotain, @xml-tools/ast, @xml-tools/common) — surfaced by pnpm's strict isolation
  • language-server now detects its bundled dist/server.js directly instead of probing node_modules (unreliable under pnpm symlinks)
  • xml-toolkit VSIX packaging reworked for pnpm (was coupled to yarn nohoist); .vscodeignore/.gitignore tightened
  • @types/vscode kept pinned at 1.56.0 (minimum supported API); rationale noted in package.json

Reviewer's guide

Review in this order:

  1. .eslintrc.js — new overrides scoped to projects/xml-tools/**, relaxing chevrotain-generated + api.d.ts violations; mirrors the vscode-logging pattern
  2. projects/xml-tools/packages/*/package.json — no private on the libraries, workspace:* deps, repository.directory/bugs point at the monorepo, files includes CHANGELOG
  3. projects/xml-tools/packages/xml-toolkit/scripts/package-vsix.js — vsce hot-patch resolves the sibling language-server via its node_modules symlink so it packages as a child; LICENSE copy is crash-safe (before pkg.json mutation)
  4. projects/xml-tools/packages/language-server/lib/api.js — SERVER_PATH detection change (only non-move logic edit)
  5. Sources under packages/*/lib & api.d.ts — source lift only, no logic changes

Not in this PR

  • VSIX runtime check — the extension is build-verified (VSIX packages with the bundled server); a manual install in a BAS dev-space to confirm XML features activate is a separate step

@rolanbadrislamov
chore(xml-tools): move sources into projects/xml-tools
01a1be9 
Raw relocation of the 10 SAP/xml-tools packages into projects/xml-tools/packages/
as-is (no logic changes). Toolchain wiring, tsconfig/eslint alignment, workspace
deps and changeset config follow in subsequent commits.

Standalone-repo root cruft (lerna.json, .husky, .github, .circleci, yarn.lock,
root package.json, root .reuse/LICENSES/LICENSE/CONTRIBUTING, scripts,
webpack.config.base.js, .eslintrc.js) is intentionally not migrated; the monorepo
root is a superset.

Signed-off-by: badrislamovrolan <rolan.badrislamov@sap.com>
@rolanbadrislamov
chore(xml-tools): prettier reflow
eaa3ecf 
Isolated formatting pass with the monorepo's Prettier, kept in its own commit
so the whole-tree reflow does not obscure the logic/wiring changes that follow.

Signed-off-by: badrislamovrolan <rolan.badrislamov@sap.com>
@rolanbadrislamov
chore(xml-tools): wire packages for pnpm monorepo build
45dedeb 
Adapt the migrated xml-tools packages to build and test under the
app-studio-toolkit pnpm workspace:

- Intra-project deps -> workspace:*; add missing transitive deps that
  yarn's flat node_modules had masked (chevrotain, @xml-tools/ast,
  @xml-tools/common).
- package.json: repository.directory + bugs pointing at the monorepo;
  drop engines.node; drop inline nyc config; list CHANGELOG in files.
- Per-package tsconfig.json (noEmit type-check) and nyc.config.js
  (lib/**/*.js @ 100%), overriding the root's src/** false-green.
- language-server: detect the bundled dist/server.js directly instead
  of keying off node_modules (unreliable under pnpm's symlinks).
- xml-toolkit: rework the VSIX packaging for pnpm (resolve the sibling
  language-server via its node_modules symlink so vsce keeps it a child;
  copy LICENSE/LICENSES from the monorepo root before mutating pkg.json;
  drop yarn nohoist coupling). Tighten .vscodeignore/.gitignore. ci runs
  clean+bundle+package (electron integration tests need a display).
- CHANGELOG.md -> CHANGELOG.old.md with a historic-note header; refresh
  READMEs to point Support/Contributing at the monorepo.

Signed-off-by: badrislamovrolan <rolan.badrislamov@sap.com>
@rolanbadrislamov
chore(xml-tools): relax eslint rules for migrated sources
b07167c 
Add inline per-project overrides for projects/xml-tools, matching the
precedent set by the earlier migrations (vscode-logging etc.): the
chevrotain-generated parser sources trip require-yield /
no-case-declarations / no-useless-escape, and the api.d.ts type
surfaces trip no-explicit-any / no-unused-vars /
no-redundant-type-constituents. Relaxed rather than rewriting migrated
code; TODO to tighten incrementally.

Signed-off-by: badrislamovrolan <rolan.badrislamov@sap.com>
@rolanbadrislamov
chore(xml-tools): add changeset fixed group for @xml-tools/* libraries
89f69dd 
The nine published @xml-tools/* packages share cross-dependencies at the
type level (ast/content-assist/etc. re-export parser & common types), so
they must publish in lockstep to avoid two versions of the same type at
runtime. The private xml-toolkit extension versions independently (its
version is the VSIX marketplace version).

Signed-off-by: badrislamovrolan <rolan.badrislamov@sap.com>
@rolanbadrislamov
chore(xml-tools): document why @types/vscode is version-pinned
5db1cef 
Signed-off-by: badrislamovrolan <rolan.badrislamov@sap.com>
@rolanbadrislamov
rolanbadrislamov marked this pull request as draft September 21, 2026 01:14
@github-actions

github-actions Bot commented Sep 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Build Report

badge

Please note:

  1. Files only stay for around 14 days!
  2. This comment will be updated with the data of the last successful build of this PR.
Name Link
Commit 40d1cb0
Logs https://github.com/SAP/app-studio-toolkit/actions/runs/35607369215
VSIX Files https://github.com/SAP/app-studio-toolkit/actions/runs/35607369215/artifacts/10642412085

@rolanbadrislamov
fix(xml-tools): correct chevrotain dep scope, xml-toolkit release, fi…
74555b5 
…les arrays

Address migration-review findings against the OSS monorepo-migration playbook:

- Move chevrotain from devDependencies to dependencies in ast, common and
  content-assist: their published api.d.ts imports IToken from chevrotain, so
  consumers' type-check needs it installed (a devDep isn't). Previously only
  worked via pnpm hoisting through @xml-tools/parser.
- Add xml-toolkit as its own fixed group in .changeset/config.json. It is a
  productive VSIX (private:true) that must still version and get a GitHub
  Release tag; being in neither fixed nor ignore was the wrong state.
- Drop .reuse and LICENSES from the 9 published packages' files arrays: those
  dirs don't exist here (xml-tools ships no per-package REUSE), so the entries
  pointed at nothing in the npm artifact.

Signed-off-by: badrislamovrolan <rolan.badrislamov@sap.com>
@rolanbadrislamov
Merge origin/main into migrate/xml-tools
df6be4f 
Resolve pnpm-lock.yaml conflict by regenerating from the merged manifest set
(pnpm install --lockfile-only), which also records the chevrotain devDep->dep
move in ast/common/content-assist. All other files auto-merged cleanly.

Signed-off-by: badrislamovrolan <rolan.badrislamov@sap.com>
@rolanbadrislamov
rolanbadrislamov marked this pull request as ready for review September 21, 2026 12:14
jacob-kreyenbuehl
jacob-kreyenbuehl reviewed Sep 21, 2026
View reviewed changes

@jacob-kreyenbuehl jacob-kreyenbuehl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed the migration end to end. Nice work, the source lift is clean and CI is green (Full Build compiles, tests, merges coverage, lints, and packages the VSIX).

What I verified (all good)

  • Source fidelity: I diffed every moved lib/test/api.d.ts against SAP/xml-tools. 242 files are byte-identical. Only 4 differ: 3 are pure prettier reflow, 1 is the documented api.js change.
  • api.js SERVER_PATH change: correct. The old check looked at node_modules existence, which does not work under pnpm (pnpm always makes a symlinked node_modules). Detecting the bundled dist/server.js directly is the right fix. bundledPath matches the webpack output.
  • VSIX packaging (package-vsix.js): well documented, and CI proves it really builds a VSIX. The sibling-via-symlink trick (so the package is a child of the extension dir) is sound.
  • Coverage: the 9 libraries run coverage:* and hit the root 100% gate. xml-toolkit runs only clean bundle package, so it opts out. This matches the pattern we use for extension backends.
  • Pinning / privacy / deps: no external consumers of @xml-tools/*, 21 workspace:* intra deps, only xml-toolkit is private, and the pnpm strict-isolation deps (chevrotain, ast, common) are declared. The @types/vscode pin has a clear reason comment.

Findings

1. Should fix (owner call): the fixed changeset group changes the version policy and would push a fake major bump to public users

The source repo uses lerna "version": "independent". The 9 packages are on purpose at different versions (ast 5.1.0, common 0.2.0, parser 1.0.11).

A fixed group forces lockstep. I checked this on the repo: both existing fixed groups (vscode-logging at 2.0.9, inquirer-gui at 3.4.13) came from sources that were already unified in lerna, so the fixed group kept their old behavior. xml-tools is the first one coming from an independent source, so here the fixed group is a real policy change.

What happens: the next changeset that touches any one package will snap all 9 to one version. So @xml-tools/common goes 0.2.0 to 5.1.x in a single release. These are public packages with heavy use (@xml-tools/ast is about 22k downloads/week), so thousands of external users get a fake 5 major jump, and every release republishes all 9 even when only 1 changed.

updateInternalDependencies: patch already bumps dependents for you, so lockstep is not needed for correctness. My rec: drop the fixed group and let them version independently, same as the source. If lockstep is what you want, it is a policy change on public packages, so please get an owner sign off first.

2. Nit: ["xml-toolkit"] as a single-item fixed group does nothing

A fixed group needs 2 or more members. task-explorer's private extension is just left out of the config and versions independently by default. Your PR text also says "xml-toolkit versions independently", which is exactly what leaving it out gives you. You can remove this line.

3. Nit: dead script reference

xml-toolkit has "vsix:prepare": "sh prepare-vsix-package.sh", but that file does not exist. Nothing in ci calls it, so it is harmless, but it is dead. Suggest removing it.

4. Needs verify (low): deprecated vsce@1.84.0

The monorepo standard is @vscode/vsce@2.24.0. It works here because the proxyquire hot patch depends on vsce 1.x internals (vsce/out/package, the ./npm getDependencies). This is fine for now because xml-toolkit has to bundle the sibling language-server dist/, which is harder than plain --no-dependencies. But it is a deprecated package with old transitive deps. Not a blocker, just worth a note for later cleanup.

Overall this is a solid, faithful migration. Finding 1 is the only real one, and it is a versioning policy question for public packages, so it needs an owner decision.

@rolanbadrislamov
chore(xml-tools): address migration review
d601cc3 
- Drop the changeset fixed group. The @xml-tools/* packages ship from an
  independent-versioned source (ast 5.1.0, common 0.2.0, parser 1.0.11);
  a fixed group would snap all nine to one version, pushing a fake major
  bump to public consumers. updateInternalDependencies:patch already
  bumps dependents, so lockstep is not needed for correctness. Let them
  version independently, as in the source repo. Also removes the no-op
  single-member ["xml-toolkit"] group.
- Remove the dead vsix:prepare script (prepare-vsix-package.sh does not
  exist and nothing in ci calls it).

Signed-off-by: badrislamovrolan <rolan.badrislamov@sap.com>
@rolanbadrislamov

rolanbadrislamov commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

Reviewed the migration end to end. Nice work, the source lift is clean and CI is green (Full Build compiles, tests, merges coverage, lints, and packages the VSIX).

FIndings 1, 2, 3 are addressed in d601cc3

jacob-kreyenbuehl
jacob-kreyenbuehl approved these changes Sep 21, 2026
View reviewed changes

@jacob-kreyenbuehl jacob-kreyenbuehl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Confirmed, both findings are resolved correctly.

  • Dropping the fixed group puts the @xml-tools/* packages back on independent versioning, same as the source repo. Verified the config is valid and xml-tools is now in neither fixed nor ignore.
  • ["xml-toolkit"] single-item group removed, and the dead vsix:prepare script is gone with no hanging references.
  • CI re-ran on the new commit and is fully green.

Thanks for addressing these cleanly. Approving.

@rolanbadrislamov
rolanbadrislamov merged commit 8fadc43 into main Sep 21, 2026
4 checks passed
@rolanbadrislamov
rolanbadrislamov deleted the migrate/xml-tools branch September 21, 2026 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jacob-kreyenbuehl jacob-kreyenbuehl jacob-kreyenbuehl approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rolanbadrislamov @jacob-kreyenbuehl