feat(envs): remove core envs from the manifest and their sources from the workspace - #10465
Open
davidfirst wants to merge 213 commits into
Open
feat(envs): remove core envs from the manifest and their sources from the workspace#10465davidfirst wants to merge 213 commits into
davidfirst wants to merge 213 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. Stale installed-aspect cache
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
…cope capsules, restore bit-aspect cmd
|
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 |
|
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
|
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
|
Code review by qodo was updated up to the latest commit 0607c7d |
- teambit.harmony/aspect and teambit.envs/env become regular envs with pinned legacy versions. components using them get the exact released behavior after bit install (the react-free aspect env rewrite is reverted) - move the bit-aspect template and harmony starters to the generator aspect so 'bit create bit-aspect' and 'bit new' work without loading the env - bind manifest deps of legacy envs to pinned versions in scope context (models built when these envs were core don't list them as dependencies) - load the full manifest graph when loading aspects from the global scope - keep legacy core env ids versionless when configured via bit create/env set
|
Code review by qodo was updated up to the latest commit b23b273 |
…ion when available - fixes the ci snap failure: pinned-version copies of workspace components leaked into the load groups and into the snap list - review fixes: index-based BFS queue in getDependentsIds, guard the typescript require in the fallback compiler, suppress legacy-env load failures only when the env package itself is missing, match both quote styles when detecting fixture env packages
|
Code review by qodo was updated up to the latest commit 94eddce |
|
Code review by qodo was updated up to the latest commit 86d1aed |
…om-manifest # Conflicts: # .bitmap # .circleci/config.yml # e2e/harmony/dependency-resolver.e2e.ts # pnpm-lock.yaml # scopes/envs/envs/environments.main.runtime.ts # scopes/harmony/aspect/babel/babel-config.ts # scopes/harmony/bit/manifests.ts # scopes/harmony/testing/load-aspect/core-aspects-ids.json
Comment on lines
+142
to
+147
| const compsToImportDepsFor = useLazyImport | ||
| ? components | ||
| : components.filter((comp) => workspaceIds.find((id) => id.isEqual(comp.id))); | ||
|
|
||
| const allDeps = (await Promise.all(compOnWorkspaceOnly.map(getDepsFunc))).flat(); | ||
| const allDepsNotImported = allDeps.filter((d) => !this.importedIds.includes(d.toString())); | ||
| const allDeps = (await Promise.all(compsToImportDepsFor.map((c) => this.getAllDepsUnfiltered(c)))).flat(); | ||
| const allDepsNotImported = allDeps.filter((d) => !this.importedIds.has(d.toString())); |
There was a problem hiding this comment.
1. Lazy import contradicts docs 🐞 Bug ⚙ Maintainability
GraphFromFsBuilder.importObjects() documents that lazy mode "only import[s] filtered dependencies" to avoid fetching huge trees, but the updated implementation always prefetches direct deps via getAllDepsUnfiltered() when shouldLoadItsDeps is set. This mismatch can mislead maintainers and can increase work when building aspects-only graphs (e.g. from WorkspaceAspectsLoader.getAspectsGraphWithoutCore).
Agent Prompt
### Issue description
`GraphFromFsBuilder.importObjects()` states that in lazy-import mode it will "only import filtered dependencies", but the current implementation collects dependencies using `getAllDepsUnfiltered()` when `useLazyImport` is true. This contradicts the method’s own docstring and the intended purpose of `shouldLoadItsDeps` (building a filtered/aspects-only graph without pulling large dependency trees).
### Issue Context
This graph builder is used by `WorkspaceAspectsLoader.getAspectsGraphWithoutCore()` to build an aspects-only graph.
### Fix Focus Areas
- scopes/workspace/workspace/build-graph-from-fs.ts[121-147]
### What to change
Choose one of the following (either is acceptable, but it should be explicit):
1) **Update the docstring/comments** to reflect the actual behavior (lazy mode batch-prefetches *unfiltered direct deps* to avoid per-dep network round-trips caused by `shouldLoadItsDeps`), *or*
2) **Change the lazy-mode dep collection** to match the documentation (only prefetch deps that pass the `shouldLoadItsDeps` filter), and ensure this doesn’t regress performance (e.g., avoid per-dep remote fetches by batching where possible).
Add/adjust a regression test (if available for graph building) asserting the intended behavior for lazy mode with a `shouldLoadItsDeps` filter.
ⓘ 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 53f7c45 |
…om-manifest # Conflicts: # .bitmap # e2e/harmony/aspect.e2e.ts # e2e/harmony/create.e2e.ts # e2e/harmony/dependency-resolver.e2e.ts # e2e/harmony/root-components.e2e.ts # pnpm-lock.yaml # scopes/envs/envs/environments.main.runtime.ts # scopes/generator/generator/templates/aspect/files/aspect-file.ts # scopes/generator/generator/templates/aspect/files/config-file.ts # scopes/generator/generator/templates/aspect/files/index.ts # scopes/generator/generator/templates/aspect/files/main-runtime.ts # scopes/generator/generator/templates/aspect/index.ts # scopes/harmony/aspect/aspect.templates.ts # scripts/e2e-test-timings.json
Comment on lines
+1209
to
+1212
| if (!envId.includes('@')) { | ||
| // versionless references hit the slot only for legacy core envs, which old components store | ||
| // without a version by design while the loaded env registers versioned. any other env must | ||
| // be looked up with its exact version: two components in the same workspace may use the |
There was a problem hiding this comment.
1. Versioned env lookup fails 🐞 Bug ≡ Correctness
EnvsMain.getEnvDefinitionById() can no longer resolve a versioned env ID to an env registered in the slot under its versionless ID, because getEnvDefinitionByStringId() only performs legacy-core fallbacks for versionless IDs. This can cause env resolution to fail (and fall back to default env / warnings) in flows where aspect-entry IDs become versioned (e.g. during tag) while the env slot entry remains versionless.
Agent Prompt
### Issue description
`EnvsMain.getEnvDefinitionById()` calls `getEnvDefinitionByStringId(id.toString())` and then `getEnvDefinitionByStringId(id.toString({ ignoreVersion: true }))`. After this PR, `getEnvDefinitionByStringId()` only performs a special lookup for *versionless* IDs (and only for legacy core envs). This means a **versioned** ID (e.g. `my-scope/my-env@1.0.0`) will not match an env that is registered in the slot under `my-scope/my-env`.
This breaks env resolution in scenarios explicitly documented in `calculateEnv()` where aspect-entry IDs can become versioned during tag even though the slot registration isn’t.
### Issue Context
The code already acknowledges that versioned IDs should match versionless slot entries (see `isEnvRegistered()`), but `getEnvDefinitionById()` / `getEnvDefinitionByStringId()` do not implement the same matching behavior.
### Fix Focus Areas
- scopes/envs/envs/environments.main.runtime.ts[1197-1220]
### Suggested fix
Implement a safe fallback for **versioned -> versionless** lookup when the exact lookup misses:
- In `getEnvDefinitionById()` (preferred):
- After failing exact match, try `id.toStringWithoutVersion()` **only if** `this.envSlot.get(id.toStringWithoutVersion())` exists, and return that `EnvDefinition`.
- Or in `getEnvDefinitionByStringId()`:
- If `envId.includes('@')` and `this.envSlot.get(envId.split('@')[0])` exists, return that.
This preserves the PR’s intent of avoiding ambiguous ignore-version scans across multiple versions, while still supporting the explicit versionless-slot contract used by core/workspace envs.
ⓘ 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 9e78b34 |
- setup_harmony: reconcile node_modules with a dev-binary install; the released bbit filters the legacy-core-env packages out of the manifest, leaving the dev binary unable to load those envs (workspace-status verification failed) - e2e_test/e2e_test_bbit: bump to medium+; installs pulling a legacy env's full closure peak over 3GB and got OOM-killed on medium - aspect.e2e.ts: install the aspect env before exercising load failures - on this branch the env is not core, so without the install the test hits an env-loading error instead of the one it verifies
|
Code review by qodo was updated up to the latest commit c289284 |
… on setup_harmony's large container
Comment on lines
+280
to
+283
| if ( | ||
| !isCompilerLessEnv && | ||
| typeof mainFile === 'string' && | ||
| /\.(ts|tsx|jsx|mts|cts)$/.test(mainFile) && |
There was a problem hiding this comment.
1. Empty-env main points dist 🐞 Bug ≡ Correctness
NodeModuleLinker.createPackageJson rewrites TS/TSX mains to dist/*.js unless it can positively detect teambit.harmony/empty-env, but linkToNodeModulesByIds loads components with loadExtensions: false so empty-env components with no explicit env config can be misdetected and get a non-existent dist main. This breaks requiring/importing such components from node_modules because only source files are linked and empty-env has no compiler to generate dists.
Agent Prompt
### Issue description
`NodeModuleLinker.createPackageJson()` rewrites a TS/TSX/etc `main` field to `dist/<main>.js` unless `isCompilerLessEnv` is true. In the `linkToNodeModulesByIds()` flow, components are loaded with `loadExtensions: false`, so `envsExt.data.id` is typically unset; for components that rely on the new default env (empty-env) and have no explicit env config, `configuredEnvId` is also unset, making `isCompilerLessEnv` false and forcing a dist main that will never exist under empty-env.
### Issue Context
This breaks consumers that import these linked packages via node resolution because the linker symlinks only bitmap/source files (not compiled `dist`), and empty-env intentionally has no compiler/dists.
### How to fix
1. Treat “no env configured / env data missing” as empty-env in this linking path (since `DEFAULT_ENV` is now `teambit.harmony/empty-env`).
2. Keep the existing safety behavior when empty-env is only a fallback for a *configured* non-empty env (i.e., if `configuredEnvId` exists and is not empty-env, do **not** treat it as compiler-less).
A concrete approach:
- Compute an `effectiveConfiguredEnvId = envsExt?.config?.env?.split('@')[0] ?? 'teambit.harmony/empty-env'`.
- Compute `effectiveDataEnvId = envsExt?.data?.id?.split('@')[0]`.
- Set `isCompilerLessEnv = (effectiveConfiguredEnvId === 'teambit.harmony/empty-env') && ((effectiveDataEnvId ?? effectiveConfiguredEnvId) === 'teambit.harmony/empty-env')`.
- Only rewrite `main` when `!isCompilerLessEnv`.
### Fix Focus Areas
- scopes/workspace/modules/node-modules-linker/node-modules-linker.ts[263-287]
- scopes/workspace/modules/node-modules-linker/node-modules-linker.ts[368-383]
ⓘ 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 baa9c2b |
…l (fetch phase OOM-killed even on 2xlarge)
Comment on lines
+724
to
+727
| const mainFile = component.state._consumer.mainFile; | ||
| if (!mainFile) return undefined; | ||
| const distMain = join(localPath, DEFAULT_DIST_DIRNAME, mainFile.replace(/\.(ts|tsx|mts|cts|jsx)$/, '.js')); | ||
| return fs.pathExistsSync(distMain) ? distMain : undefined; |
There was a problem hiding this comment.
1. Hardcoded dist main path 🐞 Bug ≡ Correctness
WorkspaceAspectsLoader.getDistMain() hardcodes the compiled main lookup to <localPath>/dist/..., so the Node 22 fallback can fail to find an existing compiled entry when an aspect/env compiler outputs to a different distDir. In that case, requiring the aspect still fails even though compiled JS exists, preventing aspect/env loading.
Agent Prompt
## Issue description
`WorkspaceAspectsLoader.getDistMain()` builds a fallback path using `DEFAULT_DIST_DIRNAME` (`dist`) and `mainFile.replace(... => .js)`. This ignores the compiler’s `getDistPathBySrcPath()` mapping, so aspects compiled to a non-`dist/` output directory won’t be found and the fallback require will still fail.
## Issue Context
`AspectLoaderMain.getAspectFilePath()` / `getRuntimePath()` already implement the correct pattern: try to obtain the component compiler and call `compiler.getDistPathBySrcPath(srcRelativePath)`, falling back to `DEFAULT_DIST_DIRNAME` only when the compiler isn't available.
## Fix Focus Areas
- scopes/workspace/workspace/workspace-aspects-loader.ts[679-728]
- scopes/harmony/aspect-loader/aspect-loader.main.runtime.ts[173-224]
ⓘ 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 bfb50f4 |
…nt inside docker OOM-kills closure installs
Comment on lines
+759
to
+762
| err.code === 'ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING' || | ||
| err.code === 'ERR_REQUIRE_ESM' || | ||
| err.message?.includes('Cannot find module') || | ||
| // a CJS dist evaluated as ESM (or vice versa) - happens when the package manager |
There was a problem hiding this comment.
1. Install masks env errors 🐞 Bug ☼ Reliability
InstallMain.reloadOneAspectsGroup() treats broad module-not-found failures from an env/aspect runtime provider as “not requirable yet” and only warns, so installs can succeed even when a non-workspace env is genuinely broken (e.g. missing runtime dependency). This can leave the workspace in a misleading “installed” state until later commands fail when the env is actually needed.
Agent Prompt
## Issue description
`InstallMain.reloadOneAspectsGroup()` suppresses provider failures for *all* reloaded aspects/envs when the error looks like module-not-found. This suppression is appropriate only for workspace aspects that may not be compiled yet during early install cycles; for non-workspace aspects (coming from scope/node_modules) a `MODULE_NOT_FOUND`/`Cannot find module` typically indicates a real broken env that should fail the install.
## Issue Context
The grouping logic explicitly creates a `scope` group for aspects not in the workspace, but the provider error suppression does not differentiate between workspace and non-workspace groups.
## Fix Focus Areas
- scopes/workspace/install/install.main.runtime.ts[744-772]
- scopes/workspace/install/install.main.runtime.ts[787-799]
## Suggested fix
1. Carry enough context into the `loadedPlugins` entries (e.g. `{ id, plugins, isWorkspace: group.workspace }`, optionally `localPath`).
2. In the provider `catch`, only apply the "not requirable yet" suppression when `group.workspace === true` (and possibly `group.envOfAspect === true`).
3. For `group.workspace === false` (scope/node_modules aspects), rethrow `MODULE_NOT_FOUND` / `Cannot find module` errors so `bit install` fails fast with an actionable message.
4. (Optional) If you still need some tolerance for non-workspace aspects, narrow it to very specific transient cases (e.g. ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING for a TS main) rather than the generic "Cannot find module" substring.
ⓘ 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 81d5756 |
|
Code review by qodo was updated up to the latest commit 88179c7 |
…fetching the full dependency universe
|
Code review by qodo was updated up to the latest commit 1e4e994 |
…obs actually reuse it
Comment on lines
+834
to
+836
| const localPath = await this.resolveInstalledAspectRecursively(component, rootIds, graph, { | ||
| throwOnError: opts.throwOnError && isRequested, | ||
| }); |
There was a problem hiding this comment.
1. Stale installed-aspect cache 🐞 Bug ☼ Reliability
getInstalledAspectResolver() now suppresses errors for non-requested dependency aspects (throwOnError gated by requestedIds), but resolveInstalledAspectRecursively memoizes failures as null and returns the cached null on later attempts. If an env/aspect package becomes available later in the same process (e.g. during multi-cycle bit install), the loader won’t retry resolution and the aspect can remain unresolved until cache invalidation/restart.
Agent Prompt
## Issue description
`WorkspaceAspectsLoader.resolveInstalledAspectRecursively()` caches failed resolutions as `null` in `resolvedInstalledAspects`. This PR also introduces a path where dependency aspects are resolved with `throwOnError: false` (based on `requestedIds`), so transient resolution failures during install can be cached and then never retried after packages are installed in the same process.
## Issue Context
- The installed-aspect resolver memoizes both successes and failures.
- Workspace cache clearing (`workspace.clearCache`) does not clear `resolvedInstalledAspects`.
- During `bit install` (and other multi-stage flows), aspects may become resolvable after `node_modules` changes, but the loader will still return cached `null`.
## Fix Focus Areas
- Add an explicit invalidation method on `WorkspaceAspectsLoader` (e.g. `clearResolvedInstalledAspectsCache()`), and call it from `Workspace.clearCache()` (and/or other places that mutate/refresh node_modules, such as post-install hooks).
- Alternatively, avoid caching `null` (or cache it only for the duration of a single load trace), so subsequent attempts can re-resolve after installation.
### Code references
- scopes/workspace/workspace/workspace-aspects-loader.ts[823-937]
- scopes/workspace/workspace/workspace.ts[871-890]
ⓘ 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 4eeff58 |
…ore-dir is overridden programmatically)
|
Code review by qodo was updated up to the latest commit 9720e5c |
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 env aspects (
teambit.react/react,teambit.harmony/node,teambit.harmony/aspect,teambit.envs/env,teambit.mdx/mdx,teambit.mdx/readme) from the core manifest to slim Bit. They now act like any other env, installed from the registry.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/envare removed like the rest, with zero behavior change. Their implementation is untouched (react-based, preview and all) - users get the exact released behavior afterbit install(the pinned-version machinery auto-installs them). New envs are created from the bitdev env packages (bit create react-envetc.), so these built-in envs are legacy surface. Thebit-aspecttemplate and the harmony starters moved to the core generator aspect, sobit create bit-aspectandbit newkeep working out of the box (the created aspect needsbit installbefore it loads, like any env).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). Keeping them versionless is deliberate on two counts. First, it keeps the env from becoming a dependency edge of its own 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. Second, it preserves forward compatibility: a re-tag under the new bit keeps the env id versionless, so a teammate who has not upgraded yet (whose bit still ships these as core) can import the re-tagged component and resolve the env - instead of receiving a versioned id their bit has no component for. The alternative (showing the component as modified and pinning the env on the next tag) would silently break not-yet-upgraded consumers.
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). Old components load without being reported as modified, and re-tagging keeps the env versionless - covered end-to-end bye2e/harmony/legacy-core-env-back-compat.e2e.ts, which imports a component exported by a pre-removal bit (env saved versionless) and asserts it is not modified and stays versionless after a re-tag.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); this path also loads the full manifest graph, and binds manifest deps of legacy envs to their pinned versions (models built when these envs were core don't list them as dependencies). The e2esetCustomEnvhelper installs the env package the fixture imports (e.g.@teambit/node).Also removes the former-core env sources from this repo's workspace (
scopes/harmony/node,scopes/react/react,scopes/harmony/aspect,scopes/envs/env,scopes/mdx/mdx,scopes/docs/readme) - bit now dogfoods them as installed packages like any consumer, and the source-vs-installed duality is gone. Making this pass end-to-end surfaced several general fixes that ride along:.docs.mdximports are detected even when the mdx aspect isn't loaded (latent gap once mdx is no longer core - without it, docs deps silently drop from dependency computation and preview bundling fails).Module._extensionsrequire hooks are restored after each build task. An in-process tester leaves@babel/register's pirates hook installed; the hook claims all.jsfiles (including node_modules, regardless of babelignoreconfig) and breaksrequire()of ESM-only packages in every later task in the process (pirates drops theformatarg node >=22.12 uses to routerequire(esm)).import()instead of a top-level require, immune to the same stale-hook hazard.@bit-no-check; timings manifest covers the split spec files so shard balancing accounts for the heavier env-install suites.