feat: bit update should update dependencies in env.jsonc#10128
feat: bit update should update dependencies in env.jsonc#10128zkochan wants to merge 18 commits intoteambit:masterfrom
Conversation
zkochan
force-pushed
the
update-env-jsonc
branch
from
fc8d0a2 to
c9dd8ce
Compare
There was a problem hiding this comment.
Pull request overview
This pull request adds support for updating dependencies in env.jsonc files when running the bit update command. Previously, bit update only updated dependencies in workspace.jsonc and component configurations, but did not handle dependencies defined in environment component's env.jsonc files.
Key changes:
- Introduces a new
jsonc-utilscomponent for parsing and modifying JSONC files while preserving formatting - Extends dependency resolution to include dependencies from env.jsonc files
- Implements logic to update env.jsonc files with new dependency versions, including special handling for peerDependencies' supportedRange
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| scopes/toolbox/json/jsonc-utils/jsonc-utils.ts | New utility functions for parsing, stringifying, and updating JSONC files while preserving formatting (indentation, newlines, comments) |
| scopes/toolbox/json/jsonc-utils/index.ts | Exports for the new jsonc-utils component |
| scopes/toolbox/json/jsonc-utils/jsonc-utils.docs.mdx | Comprehensive documentation for the jsonc-utils utility with examples and use cases |
| scopes/dependencies/dependency-resolver/get-all-policy-pkgs.ts | Adds 'env-jsonc' as a new CurrentPkgSource type to track where dependencies are defined |
| scopes/dependencies/dependency-resolver/dependency-resolver.main.runtime.ts | Implements getEnvJsoncPolicyPkgs() to extract dependencies from env.jsonc files and integrates them into the dependency resolution flow |
| scopes/workspace/install/install.main.runtime.ts | Implements updateEnvJsoncPolicies() to write updated dependency versions back to env.jsonc files and integrates it into the update workflow |
| scopes/workspace/install/pick-outdated-pkgs.ts | Updates unique name generation and adds context rendering for env-jsonc dependencies in the interactive selection UI |
| scopes/workspace/install/pick-outdated-pkgs.spec.ts | Updates test expectations to match new unique naming scheme with index suffixes |
| e2e/harmony/dependencies/env-jsonc-policies.e2e.ts | Adds end-to-end tests verifying that bit update correctly updates env.jsonc files and handles supportedRange for peerDependencies |
| .bitmap | Registers the new json/jsonc-utils component in the workspace |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scopes/dependencies/dependency-resolver/dependency-resolver.main.runtime.ts
Outdated
Show resolved
Hide resolved
scopes/dependencies/dependency-resolver/dependency-resolver.main.runtime.ts
Outdated
Show resolved
Hide resolved
scopes/dependencies/dependency-resolver/dependency-resolver.main.runtime.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scopes/dependencies/dependency-resolver/dependency-resolver.main.runtime.ts
Outdated
Show resolved
Hide resolved
scopes/dependencies/dependency-resolver/dependency-resolver.main.runtime.ts
Outdated
Show resolved
Hide resolved
scopes/dependencies/dependency-resolver/dependency-resolver.main.runtime.ts
Outdated
Show resolved
Hide resolved
scopes/dependencies/dependency-resolver/dependency-resolver.main.runtime.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scopes/dependencies/dependency-resolver/dependency-resolver.main.runtime.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scopes/dependencies/dependency-resolver/dependency-resolver.main.runtime.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scopes/dependencies/dependency-resolver/dependency-resolver.main.runtime.ts
Show resolved
Hide resolved
scopes/dependencies/dependency-resolver/dependency-resolver.main.runtime.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| getEnvJsoncPolicyPkgs(components: Component[]): CurrentPkg[] { | ||
| const policies = [ | ||
| { field: 'peers', targetField: 'peerDependencies' as const }, | ||
| { field: 'dev', targetField: 'devDependencies' as const }, | ||
| { field: 'runtime', targetField: 'dependencies' as const }, | ||
| ]; | ||
| const pkgs: CurrentPkg[] = []; | ||
| for (const component of components) { | ||
| const isEnv = this.envs.isEnv(component); | ||
| if (!isEnv) continue; | ||
|
|
||
| const envJsoncFile = component.filesystem.files.find((file) => file.relative === 'env.jsonc'); | ||
| if (!envJsoncFile) continue; | ||
|
|
||
| let envJsonc: EnvJsonc; | ||
| try { | ||
| envJsonc = parse(envJsoncFile.contents.toString()) as EnvJsonc; | ||
| } catch (error: unknown) { | ||
| const errorMessage = error instanceof Error ? error.message : String(error); | ||
| this.logger.warn(`Failed to parse env.jsonc for component ${component.id.toString()}: ${errorMessage}`); | ||
| continue; | ||
| } | ||
| if (!envJsonc.policy) continue; | ||
|
|
||
| for (const { field, targetField } of policies) { | ||
| const deps: EnvJsoncPolicyEntry[] = envJsonc.policy?.[field] || []; | ||
| for (const dep of deps) { | ||
| pkgs.push({ | ||
| name: dep.name, | ||
| currentRange: dep.version, | ||
| source: 'env-jsonc', | ||
| componentId: component.id, | ||
| targetField, | ||
| }); | ||
| } |
There was a problem hiding this comment.
getEnvJsoncPolicyPkgs() returns currentRange: dep.version for env.jsonc policy entries. When an env.jsonc peer entry uses the special version "+" (supported elsewhere in the codebase/tests), bit update will treat it as an outdated range and then updateEnvJsoncPolicies() will overwrite the "+" with an explicit version/range, changing the meaning of the policy.
Consider either skipping env.jsonc entries whose version is "+"/"-" from outdated detection, or resolving them to an actual semver range (e.g. for peers: use supportedRange) and, on update, keep version: "+" while updating the appropriate range field.
| * Update env.jsonc policy files for environment components based on a list of outdated packages. | ||
| * @param outdatedPkgs - List of outdated packages. | ||
| */ | ||
| async updateEnvJsoncPolicies(outdatedPkgs: MergedOutdatedPkg[]): Promise<void> { |
There was a problem hiding this comment.
updateEnvJsoncPolicies is only called internally from updateDependencies, but it is currently declared as a public method. If it’s not intended to be part of the Install aspect public API, mark it private (or prefix with _ consistently) to avoid expanding the public surface area unintentionally.
| async updateEnvJsoncPolicies(outdatedPkgs: MergedOutdatedPkg[]): Promise<void> { | |
| private async updateEnvJsoncPolicies(outdatedPkgs: MergedOutdatedPkg[]): Promise<void> { |
| expect(newSupportedRange).to.include(' || '); | ||
| expect(newSupportedRange).to.include('^16.8.0'); | ||
| expect(newSupportedRange).to.include(newVersion); | ||
| }); |
There was a problem hiding this comment.
The new bit update e2e coverage validates updating explicit env.jsonc versions, but it doesn’t cover the existing env.jsonc special case where a peer entry has version: "+" (resolved via supportedRange). With the new update flow, this case is at risk of being rewritten to an explicit version/range.
Add an e2e assertion that running bit update does not overwrite version: "+" entries (and define the expected behavior for updating supportedRange, if applicable).
| }); | |
| }); | |
| it('should not overwrite peer version "+" and should update supportedRange to include the new version', () => { | |
| const envId3 = 'react-based-env-peers-plus'; | |
| helper.env.setCustomNewEnv(undefined, undefined, { | |
| policy: { | |
| peers: [ | |
| { | |
| name: 'react', | |
| version: '+', | |
| supportedRange: '^16.8.0', | |
| }, | |
| ], | |
| }, | |
| }, false, envId3); | |
| helper.command.install(); | |
| const envJsoncPath = path.join(helper.scopes.localPath, envId3, 'env.jsonc'); | |
| const originalEnvJsonc = fs.readJsonSync(envJsoncPath); | |
| expect(originalEnvJsonc.policy.peers[0].version).to.equal('+'); | |
| expect(originalEnvJsonc.policy.peers[0].supportedRange).to.equal('^16.8.0'); | |
| // Update react to latest while version is resolved via "+" and supportedRange | |
| helper.command.update('react --yes'); | |
| const updatedEnvJsonc = fs.readJsonSync(envJsoncPath); | |
| const updatedPeer = updatedEnvJsonc.policy.peers[0]; | |
| // The special-case "+" version must be preserved | |
| expect(updatedPeer.version).to.equal('+'); | |
| const updatedSupportedRange = updatedPeer.supportedRange; | |
| expect(updatedSupportedRange).to.not.equal('^16.8.0'); | |
| expect(updatedSupportedRange).to.include('^16.8.0'); | |
| // The updated supportedRange should include the newly installed react version | |
| const reactPkgJsonPath = resolveFrom(helper.scopes.localPath, 'react/package.json'); | |
| const reactPkgJson = fs.readJsonSync(reactPkgJsonPath); | |
| const installedVersion = reactPkgJson.version; | |
| expect(updatedSupportedRange).to.include(installedVersion); | |
| }); |
| for (const { field, targetField } of policies) { | ||
| const deps: EnvJsoncPolicyEntry[] = envJsonc.policy?.[field] || []; | ||
| for (const dep of deps) { | ||
| pkgs.push({ | ||
| name: dep.name, | ||
| currentRange: dep.version, | ||
| source: 'env-jsonc', | ||
| componentId: component.id, | ||
| targetField, | ||
| }); |
There was a problem hiding this comment.
getEnvJsoncPolicyPkgs() currently assumes envJsonc.policy[field] is an array and iterates it directly. If an env.jsonc has policy.<field> set to a non-array (or contains special versions like "+", "-", or "*" which are valid in env.jsonc), this flow can either throw at runtime (non-array) or later break the update UI/logic (semverDiff/semver.valid expect semver-ish ranges) and even cause bit update to overwrite the special placeholder semantics. Consider guarding with Array.isArray() and skipping (or resolving) entries whose version is one of the special placeholders before pushing them into CurrentPkg[].
| const depEntry = deps.find(({ name }) => name === pkg.name); | ||
| if (depEntry) { | ||
| depEntry.version = pkg.latestRange; | ||
| if (field === 'peers' && depEntry.supportedRange) { | ||
| if (!semver.intersects(pkg.latestRange, depEntry.supportedRange)) { | ||
| depEntry.supportedRange = `${depEntry.supportedRange} || ${pkg.latestRange}`; | ||
| } | ||
| } |
There was a problem hiding this comment.
updateEnvJsoncPolicies() always overwrites depEntry.version with pkg.latestRange. env.jsonc policy supports special placeholders like "+" (keep workspace version), "-" (exclude), and possibly "*"; overwriting these changes semantics and can break env behavior. Consider skipping updates for such entries (or resolving placeholders to a concrete version before computing/persisting updates) so bit update doesn’t replace placeholders with pinned ranges.
| /** | ||
| * Updates a JSONC file content while preserving its original formatting (indentation, newlines, and comments). | ||
| * | ||
| * @param originalContent - The original JSONC file content | ||
| * @param updateFn - Function that receives the parsed data and returns the updated data | ||
| * @returns The stringified updated content with preserved formatting | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * const updatedContent = updateJsoncPreservingFormatting(originalContent, (data) => { | ||
| * data.someField = 'new value'; | ||
| * return data; | ||
| * }); | ||
| * ``` | ||
| */ | ||
| export function updateJsoncPreservingFormatting<T>( | ||
| originalContent: string, | ||
| updateFn: (data: T) => T | ||
| ): string { | ||
| const { data, formatting } = parseJsoncWithFormatting(originalContent); | ||
| const updatedData = updateFn(data); | ||
| return stringifyJsonc(updatedData, formatting); |
There was a problem hiding this comment.
updateJsoncPreservingFormatting() claims to preserve comments, but that only holds if updateFn mutates and returns the parsed object from comment-json. If callers return a new plain object, comment metadata will be dropped and comments will not be preserved. Consider adjusting the API/docs to make this constraint explicit (e.g., updateFn returns void and mutates in-place, or internally merge changes back into the original parsed object).
| /** | |
| * Updates a JSONC file content while preserving its original formatting (indentation, newlines, and comments). | |
| * | |
| * @param originalContent - The original JSONC file content | |
| * @param updateFn - Function that receives the parsed data and returns the updated data | |
| * @returns The stringified updated content with preserved formatting | |
| * | |
| * @example | |
| * ```typescript | |
| * const updatedContent = updateJsoncPreservingFormatting(originalContent, (data) => { | |
| * data.someField = 'new value'; | |
| * return data; | |
| * }); | |
| * ``` | |
| */ | |
| export function updateJsoncPreservingFormatting<T>( | |
| originalContent: string, | |
| updateFn: (data: T) => T | |
| ): string { | |
| const { data, formatting } = parseJsoncWithFormatting(originalContent); | |
| const updatedData = updateFn(data); | |
| return stringifyJsonc(updatedData, formatting); | |
| function isObjectLike(value: unknown): value is Record<string, any> { | |
| return typeof value === 'object' && value !== null; | |
| } | |
| function mergeJsoncValue<T>(target: T, source: T): T { | |
| if (target === source) return target; | |
| if (Array.isArray(target) && Array.isArray(source)) { | |
| target.length = source.length; | |
| for (let i = 0; i < source.length; i += 1) { | |
| const targetItem = target[i]; | |
| const sourceItem = source[i]; | |
| target[i] = | |
| isObjectLike(targetItem) && isObjectLike(sourceItem) | |
| ? mergeJsoncValue(targetItem, sourceItem) | |
| : sourceItem; | |
| } | |
| return target; | |
| } | |
| if ( | |
| isObjectLike(target) && | |
| isObjectLike(source) && | |
| !Array.isArray(target) && | |
| !Array.isArray(source) | |
| ) { | |
| for (const key of Object.keys(target)) { | |
| if (!(key in source)) { | |
| delete target[key]; | |
| } | |
| } | |
| for (const [key, sourceValue] of Object.entries(source)) { | |
| const targetValue = target[key]; | |
| target[key] = | |
| isObjectLike(targetValue) && isObjectLike(sourceValue) | |
| ? mergeJsoncValue(targetValue, sourceValue) | |
| : sourceValue; | |
| } | |
| return target; | |
| } | |
| return source; | |
| } | |
| /** | |
| * Updates a JSONC file content while preserving its original formatting (indentation, newlines, and comments). | |
| * | |
| * If `updateFn` mutates the parsed data in place, those changes are preserved directly. | |
| * If `updateFn` returns a new object, its values are merged back into the original parsed | |
| * JSONC structure before stringification so existing comment metadata is retained where possible. | |
| * | |
| * @param originalContent - The original JSONC file content | |
| * @param updateFn - Function that receives the parsed data and either mutates it in place or returns updated data | |
| * @returns The stringified updated content with preserved formatting | |
| * | |
| * @example | |
| * ```typescript | |
| * const updatedContent = updateJsoncPreservingFormatting(originalContent, (data) => { | |
| * data.someField = 'new value'; | |
| * }); | |
| * ``` | |
| */ | |
| export function updateJsoncPreservingFormatting<T>( | |
| originalContent: string, | |
| updateFn: (data: T) => T | void | |
| ): string { | |
| const { data, formatting } = parseJsoncWithFormatting(originalContent); | |
| const updatedData = updateFn(data); | |
| const dataToStringify = | |
| updatedData === undefined || updatedData === data ? data : mergeJsoncValue(data, updatedData); | |
| return stringifyJsonc(dataToStringify, formatting); |
| it('should update supportedRange for peerDependencies when new version is outside existing range', () => { | ||
| const envId2 = 'react-based-env-peers'; | ||
| helper.env.setCustomNewEnv(undefined, undefined, { | ||
| policy: { | ||
| peers: [ | ||
| { | ||
| name: 'react', | ||
| version: '16.8.0', | ||
| supportedRange: '^16.8.0', | ||
| }, | ||
| ], | ||
| }, | ||
| }, false, envId2); | ||
| helper.command.install(); | ||
|
|
||
| const envJsoncPath = path.join(helper.scopes.localPath, envId2, 'env.jsonc'); | ||
| const originalEnvJsonc = fs.readJsonSync(envJsoncPath); | ||
| expect(originalEnvJsonc.policy.peers[0].supportedRange).to.equal('^16.8.0'); | ||
|
|
||
| // Update react to latest (which is > 16.8.0, likely 18.x) | ||
| helper.command.update('react --yes'); | ||
|
|
There was a problem hiding this comment.
This e2e uses react as the package to update. React installs are relatively heavy and can make CI slower/flakier compared to the smaller packages used elsewhere in the update tests (e.g. is-odd/is-string). Consider switching to a lightweight package with a known non-intersecting semver jump to validate the supportedRange behavior without significantly increasing install time.
Proposed Changes