feat(envs): remove core envs from the manifest, load them as regular envs#10465
Open
davidfirst wants to merge 8 commits into
Open
feat(envs): remove core envs from the manifest, load them as regular envs#10465davidfirst wants to merge 8 commits into
davidfirst wants to merge 8 commits into
Conversation
PR Summary by QodoLoad former core envs as regular registry envs with legacy version pinning
AI Description
Diagram
High-Level Assessment
Files changed (15)
|
Code Review by Qodo
1. AspectEnv.getLinter() uses ESLint
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+740
to
+742
| while (queue.length) { | ||
| const current = queue.shift() as string; | ||
| graph.inEdges(current).forEach((edge) => { |
There was a problem hiding this comment.
1. Quadratic queue traversal 🐞 Bug ➹ Performance
Workspace.getDependentsIds() now traverses iteratively but dequeues via Array.shift(), which is O(n) per dequeue and can make the traversal O(n^2) on large graphs. This can significantly slow dependents resolution on large/deep workspaces (the exact scenario the change targets).
Agent Prompt
## Issue description
`Workspace.getDependentsIds()` uses `queue.shift()` in a loop. In JS, `shift()` is O(n) because it re-indexes the array, so the traversal can degrade to O(n^2) on large graphs.
## Issue Context
This code path was explicitly changed to handle large/deep graphs without recursion overflow, so it should also avoid avoidable algorithmic slowdowns.
## Fix Focus Areas
- scopes/workspace/workspace/workspace.ts[733-754]
### Suggested fix
Use an index-based queue (or a deque implementation):
- `const queue = ids.map(...); let idx = 0; while (idx < queue.length) { const current = queue[idx++]; ... }`
This preserves FIFO semantics without O(n) dequeues.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+8
to
+18
| export function getFallbackTypescriptCompiler() { | ||
| // eslint-disable-next-line global-require | ||
| const ts = require('typescript'); | ||
| const supportedExtensions = ['.ts', '.tsx', '.jsx']; | ||
| const compilerOptions = { | ||
| module: ts.ModuleKind.CommonJS, | ||
| target: ts.ScriptTarget.ES2019, | ||
| jsx: ts.JsxEmit.React, | ||
| esModuleInterop: true, | ||
| sourceMap: false, | ||
| }; |
There was a problem hiding this comment.
2. Unguarded typescript require 🐞 Bug ☼ Reliability
getFallbackTypescriptCompiler() calls require('typescript') without handling MODULE_NOT_FOUND, so
the fallback env can crash at runtime if TypeScript is not available. This is especially risky
because the fallback compiler is intended to be used specifically before "bit install" completes.
Agent Prompt
## Issue description
The fallback compiler does `require('typescript')` with no try/catch. If `typescript` isn't resolvable in the current runtime environment, this throws immediately and defeats the purpose of having a fallback env to keep basic flows working pre-install.
## Issue Context
The repo root `package.json` does not declare `typescript`, so availability depends on other installation mechanics; the fallback path should be defensive.
## Fix Focus Areas
- scopes/envs/envs/fallback-typescript-compiler.ts[8-18]
### Suggested fix
Wrap the require in try/catch and throw a clear, actionable error (or return a no-op compiler that surfaces a recoverable issue), e.g.:
- `let ts; try { ts = require('typescript'); } catch (e) { throw new Error('Fallback compiler requires the typescript package. Please run "bit install".'); }`
Optionally, also add a lightweight log/telemetry hook to make this failure diagnosable.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…cope capsules, restore bit-aspect cmd
Comment on lines
+815
to
831
| // guard against circular dependencies in the graph | ||
| if (visiting.has(aspectStringId)) return undefined; | ||
| visiting.add(aspectStringId); | ||
| if (rootIds.includes(aspectStringId)) { | ||
| const localPath = await this.workspace.getComponentPackagePath(aspectComponent); | ||
| this.resolvedInstalledAspects.set(aspectStringId, localPath); | ||
| return localPath; | ||
| } | ||
| const parent = graph.predecessors(aspectStringId)[0]; | ||
| // use inEdges to get the immediate parent. don't use graph.predecessors() as it returns all | ||
| // the recursive predecessors, which may throw "Maximum call stack size exceeded" on big graphs | ||
| const parentEdge = graph.inEdges(aspectStringId)[0]; | ||
| const parent = parentEdge ? graph.node(parentEdge.sourceId) : undefined; | ||
| if (!parent) return undefined; | ||
| const parentPath = await this.resolveInstalledAspectRecursively(parent.attr, rootIds, graph); | ||
| const parentPath = await this.resolveInstalledAspectRecursively(parent.attr, rootIds, graph, opts, visiting); | ||
| if (!parentPath) { | ||
| this.resolvedInstalledAspects.set(aspectStringId, null); | ||
| return undefined; |
There was a problem hiding this comment.
1. Wrong parent chosen 🐞 Bug ≡ Correctness
resolveInstalledAspectRecursively() follows only the first inbound edge (single parent) and memoizes a null result on failure, so aspects with multiple parents may be incorrectly marked unresolvable even when another parent chain could resolve them from node_modules.
Agent Prompt
### Issue description
`resolveInstalledAspectRecursively()` picks `graph.inEdges(id)[0]` as the only parent to walk, and if that path fails it caches `null` in `resolvedInstalledAspects`. In graphs where a node has multiple parents (common in dependency graphs), this can incorrectly prevent resolution via a different parent chain.
### Issue Context
This function is used to resolve aspects from installed packages, and caching failures globally per-aspect makes the behavior sensitive to graph edge ordering.
### Fix Focus Areas
- scopes/workspace/workspace/workspace-aspects-loader.ts[803-850]
### Suggested fix
1. Replace the single-parent selection with a loop over *all* `graph.inEdges(aspectStringId)`.
2. For each candidate parent edge:
- Resolve `parentPath` recursively (consider passing a cloned `visiting` set per branch).
- Attempt `resolveFrom(parentPath, packageName)`.
- On first success: cache the resolved path and return it.
3. Only cache a terminal failure (`null`) after *all* parents were tried and failed.
4. Avoid caching `null` from a single parent attempt, because another parent may succeed.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit c7dd1a7 |
…cted pre-install warning, regen references
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
Comment on lines
+124
to
+137
| // break circular env chains - if an aspect is already in the process of loading (a parent | ||
| // call in the current chain), don't try to load it again. | ||
| notLoadedIds = notLoadedIds.filter((id) => !this.workspace.inFlightAspectsLoads.has(id.split('@')[0])); | ||
| if (!notLoadedIds.length) { | ||
| span.setAttribute('alreadyLoaded', true); | ||
| return []; | ||
| } | ||
| const inFlightAdded = notLoadedIds.map((id) => id.split('@')[0]); | ||
| inFlightAdded.forEach((id) => this.workspace.inFlightAspectsLoads.add(id)); | ||
| try { | ||
| return await this.loadAspectsAfterInFlightCheck(notLoadedIds, span, neededFor, mergedOpts, loggerPrefix); | ||
| } finally { | ||
| inFlightAdded.forEach((id) => this.workspace.inFlightAspectsLoads.delete(id)); | ||
| } |
There was a problem hiding this comment.
1. In-flight ignores version 🐞 Bug ☼ Reliability
WorkspaceAspectsLoader.loadAspectsWithSpan() de-duplicates in-flight loads using id-without-version
(id.split('@')[0]), so a request for foo@2.0.0 can be dropped while foo@1.0.0 is still loading and
the caller returns without ensuring the requested version is loaded. This can leave Harmony missing
required aspects or using an unintended version, causing non-deterministic runtime failures during
aspect/env resolution.
Agent Prompt
## Issue description
`WorkspaceAspectsLoader.loadAspectsWithSpan()` uses `workspace.inFlightAspectsLoads` keyed by `id.split('@')[0]` (id-without-version). This conflates different versions of the same aspect/env and can cause a request for a different version to be filtered out and never awaited/loaded by the current call.
## Issue Context
- The in-flight set is explicitly documented as storing aspect-ids **without version**.
- The loader itself acknowledges that ids may be requested **with** versions.
## Fix Focus Areas
- Make the circular-load guard version-aware for non-legacy aspects (e.g., key by full id, or use a per-call stack keyed by full id).
- If you still need “ignore version” behavior, scope it narrowly to legacy-core envs (or other explicitly single-instance cases), not globally.
- Consider replacing the Set with a `Map<fullId, Promise>` (or similar) so concurrent requests can *join/await* the in-flight load rather than silently skipping.
### Target code
- scopes/workspace/workspace/workspace-aspects-loader.ts[124-137]
- scopes/workspace/workspace/workspace-aspects-loader.ts[150-154]
- scopes/workspace/workspace/workspace.ts[210-214]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit e6418b9 |
|
Code review by qodo was updated up to the latest commit c9eca3d |
…aspect and env envs to core
…eambit/bit into remove-core-envs-from-manifest
Comment on lines
+244
to
+250
| private getFallbackDefaultEnv(): Environment { | ||
| if (!this.fallbackDefaultEnv) { | ||
| this.fallbackDefaultEnv = { | ||
| name: 'node', | ||
| getCompiler: () => getFallbackTypescriptCompiler(), | ||
| __getDescriptor: async () => ({ type: 'node' }), | ||
| }; |
There was a problem hiding this comment.
2. Fallback env reports node 🐞 Bug ≡ Correctness
When a legacy core env isn’t loaded yet, EnvsMain falls back to a minimal env whose descriptor is
hard-coded to { type: 'node' } and name: 'node', so non-node legacy envs (react/mdx/html) can
get persisted/displayed as “node”. This env descriptor is persisted into extension data during scope
component recalculation, so the misleading metadata can survive beyond the transient “env not
loaded” state.
Agent Prompt
### Issue description
Legacy core envs that are not installed/loaded fall back to `getFallbackDefaultEnv()`, but that fallback env always reports `name: 'node'` and `__getDescriptor(): { type: 'node' }`. This causes components whose configured env is *not* node (e.g. react/mdx/html legacy core envs) to get an incorrect env descriptor (type/name) that can be persisted into model extension data during component recalculation.
### Issue Context
- The fallback is returned from `EnvsMain.getEnv()` specifically for legacy core env IDs without a version.
- `EnvsMain.calcDescriptor()` is used during scope-side recalculation and then persisted via `upsertExtensionData`.
### Fix Focus Areas
- scopes/envs/envs/environments.main.runtime.ts[237-253]
- scopes/envs/envs/environments.main.runtime.ts[553-564]
- scopes/envs/envs/environments.main.runtime.ts[696-710]
- scopes/scope/scope/scope.main.runtime.ts[329-357]
### Suggested fix
Change the fallback env descriptor/name so it cannot misrepresent itself as a specific env (node). Options:
1. Make the fallback env report a neutral identity (e.g. `name: 'fallback'`, `type: 'fallback'`).
2. Preferably: make the fallback env factory accept the requested env id (without version) and return an env whose `name/type` reflect that env id (or at least `type: 'missing-env'`), while still providing the minimal TS transpiler.
Also ensure the persisted descriptor remains accurate enough for user-facing commands (show/envs/status) and doesn’t mislabel legacy react/mdx/html envs as node.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 3f5c24e |
…nv templates on demand - register legacy core env ids as core extension names so their config entries stay name-only (versionless): prevents env-as-dependency edges that created circular TS project references in lane/tag builds - require the aspect env eslint/prettier configs lazily (saves ~400 file reads per bit command) - bit create: fall back to --env for template lookup, incl. templates registered on the generator slot by envs loaded from the global scope - rewrite e2e node-env fixtures to compose on the core aspect env instead of @teambit/node
Comment on lines
+149
to
+165
| getLinter(context: LinterContext, transformers: EslintConfigTransformer[] = []): Linter { | ||
| const tsconfigPath = require.resolve('./typescript/tsconfig.json'); | ||
| const mergedOptions = { | ||
| // @ts-ignore - this is a bug in the @types/eslint types | ||
| overrideConfig: getEslintConfig() as ESLintLib.Options, | ||
| extensions: context.extensionFormats, | ||
| useEslintrc: false, | ||
| cwd: __dirname, | ||
| fix: !!context.fix, | ||
| fixTypes: context.fixTypes as ESLintLib.Options['fixTypes'], | ||
| } as ESLintOptions; | ||
| const configMutator = new EslintConfigMutator(mergedOptions); | ||
| const transformerContext: EslintConfigTransformContext = { fix: !!context.fix }; | ||
| configMutator.setTsConfig(tsconfigPath); | ||
| const afterMutation = runTransformersWithContext(configMutator.clone(), transformers, transformerContext); | ||
| return ESLintLinter.create(afterMutation.raw, { logger: this.logger }); | ||
| } |
There was a problem hiding this comment.
1. aspectenv.getlinter() uses eslint 📘 Rule violation ⚙ Maintainability
AspectEnv introduces an ESLint-backed linter (ESLintLinter) and imports eslint, which reintroduces ESLint-based linting into the repo despite the standard being Oxlint + TypeScript checks. This can create inconsistent linting behavior and increases maintenance/runtime dependency on ESLint in core tooling.
Agent Prompt
## Issue description
`AspectEnv` now provides linting via `ESLintLinter` and imports `eslint`, which conflicts with the repo compliance requirement to avoid ESLint-based linting and rely on Oxlint + TypeScript type checking.
## Issue Context
The new `getLinter()` implementation builds an ESLint options object (including `overrideConfig`) and returns `ESLintLinter.create(...)`, making ESLint part of the default tooling surface for this environment.
## Fix Focus Areas
- scopes/harmony/aspect/aspect.env.ts[15-29]
- scopes/harmony/aspect/aspect.env.ts[149-165]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 0607c7d |
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
1 participant
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.
Removes the heavy env aspects (
teambit.react/react,teambit.harmony/node,teambit.mdx/mdx,teambit.mdx/readme) from the core manifest to slim Bit. They now act like any other env: installed from the registry and configured with a version.New default env:
teambit.harmony/empty-env(core). A totally empty env - no compiler, no tester, no preview, no dependency policy. Components with no env configured use it and work fully offline out of the box (add → compile no-op → tag/snap → export). Since it has no behavior, it has nothing to drift when bit itself changes - the one env that is safe to keep core (and versionless in models) forever. To get a dev experience, users configure a real env (bit createflows already do).teambit.harmony/aspectandteambit.envs/envstay core, rewritten react-free. The aspect env is now standalone on top of the core typescript/babel/jest tooling (own tsconfig/jest/eslint/prettier configs, no react merge; template-bundler/preview dropped as they require the react stack). The env env composes from it as before. This keeps aspects and custom envs compiling/loading/typed out of the box - required for aspect development and the entire aspect/custom-env e2e surface.Versionless by design. Config entries for the removed env ids are persisted by name, without a version - exactly as they were when core (registered as core-extension names). This also means they never become dependency edges of their components; otherwise an env such as react, whose dependency closure includes components that use it as their env, creates circular TS project references and breaks lane/tag builds.
Backward compatibility. Old components have the removed envs saved without a version.
legacy-core-envs.tsmaps them to pinned versions, applied only at the resolution/loading/install level - stored objects are never mutated. Versionless legacy ids match the env slot ignoring version,bit installauto-adds their packages, and single-instance semantics are enforced (a loaded version is reused rather than loading another copy). Not-installed legacy envs fail fast with aNonLoadedEnvissue suggestingbit install- no scope-capsule isolation in workspace context (which used to take minutes).Relocated core wiring: the
bit aspectCLI command moved toteambit.workspace/workspace;validateBeforePersistHookmoved toteambit.dependencies/dependency-resolver; the dead@teambit/legacylink is now skipped instead of crashing.Also fixes latent issues this path exposed: versionless seeders filtering out all manifests in
loadExtensionsByManifests, circular env chains causing infinite component-load recursion, versioned core-aspect ids escaping core filters anddoRequiremutating shared core manifests, stack overflows from recursive graph traversal, and a spuriousMissingDistsissue for compiler-less envs.Verified locally: fresh workspace (JS and TS components) - clean status in ~1s, tag/snap/export offline,
bit envs/bit testgraceful; this repo's workspace - status/insights/list-core clean; the seven repo components that relied on the default env are now explicitly set to the node env.bit create <template> --env <removed-env>loads the env's templates on demand from the global scope (pinned version). The aspect env's eslint config chain is now required lazily (~400 fewer file reads per command). The e2e node-env fixtures compose on the core aspect env instead of@teambit/node.