Fix Node restore SDK anchor and introduce extensible Node toolchain - #104
Open
TomProkop wants to merge 20 commits into
Open
Fix Node restore SDK anchor and introduce extensible Node toolchain#104TomProkop wants to merge 20 commits into
TomProkop wants to merge 20 commits into
Conversation
…ary/CodeApp A standalone "dotnet restore" sets ExcludeRestorePackageImports=true, so NuGet never imports ordinary PackageReference/buildTransitive-delivered .targets files - including the Tasks package's NodeRestore.targets - during that operation. The per-project-type NodeRestore anchors (_PcfNodeRestore, _ScriptLibraryNodeRestore, _CodeAppNodeRestore) lived in exactly those excluded files, so a bare restore silently skipped Node/rush hydration entirely. Moves the anchor into Sdk.targets, which is imported through the MSBuild SDK-resolver mechanism and is therefore not subject to ExcludeRestorePackageImports. Scoped to Pcf/ScriptLibrary/CodeApp only, since every other project type also references this SDK for unrelated shared targets (e.g. GenerateVersionNumber) but must never attempt Node/rush restore. Two additional issues surfaced during real restore testing against a live PCF control, both fixed here: - $(NuGetPackageRoot) is not reliably populated in the nested MSBuild evaluation NuGet uses internally to build the restore graph, silently preventing the anchor from ever firing. Sdk.props now derives the NuGet package cache root from its own resolved file location instead. - On a brand new machine/cache, the Tasks package isn't downloaded yet when the anchor's nested evaluation runs, so Node deps were never hydrated by that restore at all. A second hook, AfterTargets="Restore", re-invokes the anchor in a freshly re-evaluated nested build once the outer restore has finished downloading packages. Validated end-to-end against FileExplorer (a real, complex PCF control) with disposable local test packages: a single cold-cache "dotnet restore" now hydrates node_modules via rush, a warm-cache restore is idempotent, a normal dotnet build has no duplicate-import warnings, and a Solution-type project restore correctly triggers zero Node/rush activity for itself. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sdk.targets previously mixed four unrelated concerns in one file: Microsoft.NET.Sdk passthrough, per-project-type PackageReference wiring, Git-versioning defaults, and the NodeRestore restore-time anchor. Split into: - Sdk.targets: thin entry point, imports the three files below. - Sdk.PackageReference.targets: wires in the per-project-type Dataverse package based on $(ProjectType). - Sdk.GitVersioning.targets: Git-based version-number defaults + ResolveGitBranch. - Sdk.NodeRestore.targets: the restore-time Node dependency hydration anchor, with a top-of-file note clarifying it's restore-time only - build-time Node delegation lives in each project type's own .targets file and the Tasks package's NodeBuild targets. Pure reorganization, no logic changes. Validated: dotnet build 0 warnings/0 errors; cold-cache restore against a disposable FileExplorer test project hydrates node_modules via rush; warm restore is idempotent (~2s, no-op); full dotnet build succeeds with 0 warnings (no MSB4011 duplicate-import regression); Solution-type negative test confirms NodeRestore never fires for non-Node project types. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Follow-up to the Sdk.targets split - the one-line comments in Pcf/ScriptLibrary/CodeApp's own .targets files pointed at Sdk.targets, which no longer directly contains _NodeRestoreAnchor after the split. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TomProkop
force-pushed
the
fix/noderestore-sdk-resolver-anchor
branch
3 times, most recently
from
August 3, 2026 10:39
07e1a91 to
4a76129
Compare
TomProkop
changed the title
TomProkop
force-pushed
the
fix/noderestore-sdk-resolver-anchor
branch
from
August 3, 2026 11:05
4a76129 to
d5fecf5
Compare
…ectDirectory, scope Rush restore - NodeRootPath defined in shared ProjectPaths.props (applies to all Node-based project types: Pcf, ScriptLibrary, CodeApp) - Follows SolutionRootPath pattern: relative path (default .), resolved to NodeRootFullPath via GetFullPath - NodeRestoreProjectDirectory removed — NodeRootFullPath used directly throughout the NodeRestore system (no redundant indirection) - Backward compat: TypeScriptDir still accepted if NodeRootPath is not set - Rush restore scoped conditionally: --to . for project-level restores, unscoped for solution-level restores (detected via SolutionPath) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TomProkop
force-pushed
the
fix/noderestore-sdk-resolver-anchor
branch
from
August 3, 2026 13:13
d5fecf5 to
987002a
Compare
MSBuild sets $(SolutionPath) to the literal string "*Undefined*" when not building from a solution, rather than leaving it empty. The previous condition only checked for empty, so project-level restores never got the --to . scope. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Not ready to approve
There’s a confirmed backward-compat risk where NodeRootFullPath is computed in a way that can break absolute-path overrides carried over from TypeScriptDir, plus a couple of documentation inaccuracies introduced by the refactor.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR fixes restore-time Node dependency hydration for Node-based Dataverse project types by anchoring NodeRestore from the SDK-resolved targets (so it runs during a bare dotnet restore), while also refactoring the SDK targets layout and standardizing the Node project root property name.
Changes:
- Add
Sdk.NodeRestore.targetswith a restore-time_NodeRestoreAnchor(plus cold-cache follow-up) and splitSdk.targetsinto focused imports. - Rename
TypeScriptDir→NodeRootPath(with compatibility mapping) and update PCF/ScriptLibrary/CodeApp to use$(NodeRootFullPath). - Scope Rush restore (
rush update/install) based on solution-level vs project-level invocation.
File summaries
| File | Description |
|---|---|
| src/Sdk/Sdk/Sdk.targets | Split into thin entry point importing per-concern target files. |
| src/Sdk/Sdk/Sdk.props | Derive _TALXISDevKitNuGetPackageRoot from resolved SDK path for restore-time imports. |
| src/Sdk/Sdk/Sdk.PackageReference.targets | New file: centralizes per-ProjectType Dataverse package reference wiring. |
| src/Sdk/Sdk/Sdk.GitVersioning.targets | New file: isolates Git versioning defaults and ResolveGitBranch. |
| src/Sdk/Sdk/Sdk.NodeRestore.targets | New file: anchors NodeRestore during restore using SDK-resolved imports, with cold-cache handling. |
| src/Dataverse/Tasks/msbuild/tasks/Targets/NodeRestore/Rush.targets | Use NodeRootFullPath and add Rush restore scoping (--to .) when not building from a solution. |
| src/Dataverse/Tasks/msbuild/tasks/Targets/NodeRestore/Generic.targets | Switch package.json path resolution to NodeRootFullPath. |
| src/Dataverse/Tasks/msbuild/tasks/Targets/NodeRestore/Detection.targets | Detection now walks upward from NodeRootFullPath instead of NodeRestoreProjectDirectory. |
| src/Dataverse/Tasks/msbuild/tasks/Targets/NodeRestore/CustomCommand.targets | Run custom restore commands from NodeRootFullPath. |
| src/Dataverse/Tasks/msbuild/tasks/Targets/NodeRestore.targets | Update docs/comments to reflect NodeRootPath/NodeRootFullPath model. |
| src/Dataverse/Tasks/msbuild/tasks/Props/ProjectPaths.props | Introduce NodeRootPath + legacy TypeScriptDir mapping and compute NodeRootFullPath. |
| src/Dataverse/Tasks/msbuild/buildTransitive/TALXIS.DevKit.Build.Dataverse.Tasks.targets | Mark Tasks import as already-loaded to avoid duplicate import warnings. |
| src/Dataverse/ScriptLibrary/README.md | Document NodeRootPath in place of TypeScriptDir. |
| src/Dataverse/ScriptLibrary/msbuild/tasks/TALXIS.DevKit.Build.Dataverse.ScriptLibrary.targets | Update ScriptLibrary Node paths to NodeRootFullPath and rely on SDK restore anchor. |
| src/Dataverse/Pcf/msbuild/tasks/TALXIS.DevKit.Build.Dataverse.Pcf.targets | Remove per-type restore anchor; use NodeRootFullPath for Node build directory. |
| src/Dataverse/CodeApp/msbuild/tasks/TALXIS.DevKit.Build.Dataverse.CodeApp.targets | Remove per-type restore anchor; use NodeRootFullPath consistently for Node paths. |
| docs/NodeDependencies.md | Replace TypeScriptDir mention with NodeRootPath/NodeRootFullPath documentation. |
| docs/BuildProcess.md | Update ScriptLibrary narrative to reference NodeRootFullPath (but hook list needs alignment). |
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
- Use MSBuild::NormalizePath instead of GetFullPath for NodeRootFullPath so absolute-path overrides (from TypeScriptDir or explicit NodeRootPath) are handled correctly. - Remove stale _ScriptLibraryNodeRestore hook from BuildProcess.md (now handled by _NodeRestoreAnchor in Sdk.NodeRestore.targets). - Fix NodeDependencies.md detection description to reflect all project types walk from NodeRootFullPath (not just ScriptLibrary). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
zekelinAlex
reviewed
Aug 3, 2026
zekelinAlex
left a comment
zekelinAlex
left a comment
Collaborator
There was a problem hiding this comment.
Took this for a proper spin today (INT0015, the TALXIS repo and a pd-package sandbox). The idea is right, but I hit four issues while testing. All of them are fixed on the branch now:
- restore died for a ScriptLibrary/CodeApp without package.json: the per-type RunNodeBuild defaults aren't imported during a bare restore, so the anchor ran npm in a folder with nothing to install. The anchor now probes for package.json itself (98cc00e).
- fresh clone + solution-level restore couldn't hydrate Node deps: NuGet runs no per-project Restore target at solution scope, so the AfterTargets="Restore" fallback never fires there. NodeRestore now also sits in each type's build chain, so the next build (even with --no-restore) self-heals; the remaining one-restore gap is documented (2ad04b8).
- the rush restore scoping didn't actually work: "rush update" has no --to in any rush version, and "install --to ." ran from the repo root where "." can't resolve. Scoping now engages only on a never-installed workspace, runs "install --to ." from the project directory and falls back to a full update outside CI (03f76dd).
- the big one: a solution-scope restore fires the anchor for every project in parallel, and rush's pre-lock phases (pnpm bootstrap in ~/.rush, lockfile copies in common/temp) corrupt each other under concurrency. On a 124-project rush repo that meant 137 errors and a trashed pnpm cache. Rush invocations are now serialized behind a per-workspace mutex, an up-to-date gate keeps warm restores at zero rush spawns, and a stale last-install.flag gets cleared when a project's node_modules were deleted (03f76dd).
Verified end to end: INT0015 (fresh hydration, warm restore ~15-30s with no rush processes, delete node_modules then restore self-heals), a full TALXIS publish over the legacy TypeScriptDir layout, and a pd-package sandbox rebuilt from git clean. Docs updated to match (cf63ef9). CodeApp and the npm ci path weren't covered by this pass.
zekelinAlex
approved these changes
Aug 3, 2026
- bound mutex names with a stable hash and reduce retry duration - parse Rush project/subspace configuration with a typed MSBuild task - support project dependency selection across standard Rush subspaces - preserve the warm gate while tracking shared Rush inputs - rely on Rush to self-heal project links without deleting its state Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Rename SelectNodeAdapter → SelectToolAdapter (ecosystem-agnostic) - Rename Detection.targets → Selection.targets, _NodeRestoreResolve → _NodeRestoreSelect - Split Generic.targets → Npm.targets, Pnpm.targets, Yarn.targets, Bun.targets - Rename Rush targets to tool-first convention: _NodeRestoreRushDetect/Resolve/Run - Rename build targets: _NodeBuildDirect, _NodeBuildDelegateToRush - Rename retry targets: _NodeExecWithRetry, _NodeExecRetry*, _NodeExecRushLockMessage - Switch NodeBuild.targets from wildcard to explicit imports - Add docs/MSBuildConventions.md documenting naming rules and adapter pattern Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TomProkop
changed the title
- Extract CI detection into Props/CIDetection.props (single source of truth) - Remove NodeRestoreAdapterConfigureDependsOn and RunDependsOn extension points - Remove _NodeRestoreAdapterConfigurePhase and RunPhase aggregator targets - Tool resolve/run targets use AfterTargets="_NodeRestoreSelect" (standard MSBuild) - Built-in and external NuGet adapters now follow the same pattern - Fix Bun lockfile detection: support both bun.lock (v1.2+) and bun.lockb (legacy) - Fix Yarn Berry CI: just 'yarn install' (Berry auto-detects CI, no --immutable needed) - NodeRestore entry point simplifies to DependsOnTargets="_NodeRestoreSelect" Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
zekelinAlex
self-requested a review
August 10, 2026 10:23
Separate package-manager and orchestrator detection from restore and build lifecycles, remove the transitional adapter architecture, and centralize CI detection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TomProkop
changed the title
Promote candidate and selected items to public MSBuild names, add the public NodeToolchain target, and keep built-in providers from claiming third-party package managers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep existing ScriptLibrary projects on TypeScriptDir without migration and document its unchanged normalized path semantics across normal and cold-cache restore evaluation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Document NodeRootPath as the sole configuration surface while retaining a time-bounded compatibility bridge in implementation comments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Split package-manager build providers, expose structured build arguments, and move Rush lifecycle eligibility into detection so every provider consumes the same selected-item contract. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
Summary
Fixes #90. Fixes #93.
Bare
dotnet restorecan now hydrate Node dependencies even though the Tasks package's build-transitive imports are excluded during NuGet restore. The SDK provides the restore anchor and re-evaluates cold-cache projects after NuGet downloads the Tasks package.This also replaces the transitional single-adapter design with an extensible Node toolchain boundary that models package managers and orchestrators as independent roles.
Architecture
NodeToolchaindetects and resolves one package manager (npm,pnpm,yarn, orbun) and an optional orchestrator (rush) independently.ResolveNodeToolchainperforms Node-specific priority selection, duplicate/tie validation, explicit override handling, and preserves candidate metadata.NodeToolchain/*owns detection and provider-specific eligibility, including Rush project/topology evaluation.NodeRestore/*owns dependency hydration only.NodeBuild/*owns build execution only.rush.json; unregistered projects retain a non-owning Rush selection and fall back to their selected package manager.NodePackageManagerCandidateorNodeOrchestratorCandidateitems and hook the publicNodeRestore/NodeBuildlifecycles with standard MSBuild target hooks. The selected items are exposed asNodeSelectedPackageManagerandNodeSelectedOrchestrator, with provider metadata preserved. Built-in providers use the same contract.NodeBuildArgumentitems carry project-type arguments to every built-in or external build provider; Rush validates its forwarded arguments from optionalRushParameterNamemetadata.The file and target names now map directly to their modules, for example:
Restore and Rush behavior
bun.lockandbun.lockb).NodeRestoreCommandremains an exact command override and suppresses built-in restore providers.Compatibility and breaking changes
NodeRootPathis the Node project-root property across all Node project types.NodeRestoreProjectDirectory.NodePackageManager=rush; Rush is configured throughNodeOrchestrator.SelectToolAdapter, adapter candidates/phases, and adapter terminology).CIDetection.props;GenerateGitVersionconsumes the resolvedIsRunningInCIvalue.Validation
Noneroles, custom restore commands, public build arguments, Rush+pnpm independent roles, registered Rush subspaces, unregistered non-owning Rush fallback, and external package-manager/orchestrator providers consuming only the public contract.NodeToolchain,NodeRestore, andNodeBuildfiles and no replaced adapter files.