feat(envs): remove core envs from the manifest and their sources from the workspace#10465
Open
davidfirst wants to merge 196 commits into
Open
feat(envs): remove core envs from the manifest and their sources from the workspace#10465davidfirst wants to merge 196 commits into
davidfirst wants to merge 196 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. Wrong aspect path resolution
|
![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 |
…v source removed)
|
Code review by qodo was updated up to the latest commit 86ede67 |
…y not-yet-compiled reloads
…om-manifest # Conflicts: # .bitmap
|
Code review by qodo was updated up to the latest commit 50d29a5 |
|
Code review by qodo was updated up to the latest commit 3ebce3b |
…om-manifest # Conflicts: # .bitmap # pnpm-lock.yaml
|
Code review by qodo was updated up to the latest commit 3d8f969 |
Comment on lines
+403
to
+423
| /** | ||
| * same as getTemplateWithId, but when the template is not found and --env was provided, try | ||
| * loading the template from that env. needed for envs that used to be core aspects with | ||
| * built-in templates, e.g. "bit create react comp1 --env teambit.react/react". such envs are | ||
| * loaded on-demand, and legacy core env ids are resolved to their pinned version. | ||
| */ | ||
| private async getTemplateWithIdOrEnvFallback( | ||
| templateName: string, | ||
| aspect?: string, | ||
| env?: string | ||
| ): Promise<ComponentTemplateWithId> { | ||
| try { | ||
| return await this.getTemplateWithId(templateName, aspect); | ||
| } catch (err) { | ||
| if (!env || env === aspect) throw err; | ||
| try { | ||
| return await this.getTemplateWithId(templateName, resolveLegacyCoreEnvId(env)); | ||
| } catch { | ||
| throw err; // the original "template not found" error is more relevant | ||
| } | ||
| } |
There was a problem hiding this comment.
1. Env fallback catches all 🐞 Bug ≡ Correctness
GeneratorMain.getTemplateWithIdOrEnvFallback() catches any error from getTemplateWithId() and then retries template lookup using the component --env value as an aspect id. This can mask real template-loading errors and can unintentionally source templates from arbitrary envs, despite the comment saying the fallback is intended specifically for legacy ex-core env ids.
Agent Prompt
### Issue description
`getTemplateWithIdOrEnvFallback()` falls back to `resolveLegacyCoreEnvId(env)` on *any* thrown error from `getTemplateWithId()`, not just "template not found". This can hide real failures (e.g. aspect load errors, runtime exceptions during template resolution) and can make `--env` influence template resolution even when it’s just meant to override the generated component’s env.
### Issue Context
The CLI help states `--env` is for setting the component’s environment, while `--aspect/--template` controls the template source.
### Fix Focus Areas
- scopes/generator/generator/generator.main.runtime.ts[392-423]
- scopes/generator/generator/generator.main.runtime.ts[451-461]
- scopes/generator/generator/create.cmd.ts[63-70]
### Suggested fix direction
- Only attempt the env-based fallback when the original failure is specifically a template-not-found condition (e.g. `err instanceof BitError && err.message === ...` or a dedicated error type).
- Further restrict fallback to the intended case from the comment: only when `env` is one of the legacy ex-core env IDs (or `envs.isLegacyCoreEnv(env.split('@')[0])` if GeneratorMain can access EnvsMain).
- Consider requiring `aspect` to be unset for this fallback (so `--aspect` remains authoritative).
- Keep rethrowing the original error when fallback fails (current behavior already does this).
ⓘ 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 9b931b8 |
…, not only legacy
Comment on lines
+34
to
+45
| const replaceFileExtToJs = (filePath: string): string => filePath.replace(/\.(ts|tsx|jsx|mts|cts)$/, '.js'); | ||
| return { | ||
| id: 'teambit.envs/envs/fallback-typescript-compiler', | ||
| displayName: 'Fallback TypeScript', | ||
| distDir: 'dist', | ||
| shouldCopyNonSupportedFiles: true, | ||
| displayConfig: () => JSON.stringify(compilerOptions, null, 2), | ||
| version: () => ts.version as string, | ||
| isFileSupported: (filePath: string) => | ||
| supportedExtensions.some((ext) => filePath.endsWith(ext)) && | ||
| !['.d.ts', '.d.mts', '.d.cts'].some((ext) => filePath.endsWith(ext)), | ||
| getDistPathBySrcPath: (srcPath: string) => `dist/${replaceFileExtToJs(srcPath)}`, |
There was a problem hiding this comment.
1. Fallback mangles .d.ts 🐞 Bug ≡ Correctness
The new fallback TypeScript compiler rewrites declaration filenames like *.d.ts into dist/*.d.js because getDistPathBySrcPath() applies a .ts -> .js replacement to any path ending with .ts. When the scope aspects loader copies non-supported files (including .d.ts) via compiler.getDistPathBySrcPath(outputPath), the declaration is emitted under the wrong filename and the expected .d.ts is missing from dist.
Agent Prompt
## Issue description
The fallback compiler’s `replaceFileExtToJs()` and `getDistPathBySrcPath()` rewrite any path ending in `.ts`, which also matches declaration files like `index.d.ts`, producing `index.d.js`. This causes the fallback compilation/copy path to emit misnamed declaration files in `dist`.
## Issue Context
`ScopeAspectsLoader.compileIfNoDist()` copies non-supported files (including `.d.ts`) by passing their original `outputPath` through `compiler.getDistPathBySrcPath()`. The fallback compiler excludes `.d.ts` from `isFileSupported()`, so declarations will commonly go through this “copy as-is” branch.
## Fix Focus Areas
- scopes/envs/envs/fallback-typescript-compiler.ts[34-45]
### Implementation notes
- Update `replaceFileExtToJs()` (or `getDistPathBySrcPath()`) to preserve declaration suffixes: `.d.ts`, `.d.mts`, `.d.cts`.
- Example approach: early-return the original path when it ends with those declaration extensions; otherwise apply the `.ts/.tsx/.jsx/.mts/.cts -> .js` rewrite.
ⓘ 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 71fe0d4 |
Comment on lines
+898
to
+901
| if (this.workspace.hasId(aspectComponent.id, { ignoreVersion: true })) { | ||
| const localPath = await this.workspace.getComponentPackagePath(aspectComponent); | ||
| this.resolvedInstalledAspects.set(aspectStringId, localPath); | ||
| return localPath; |
There was a problem hiding this comment.
1. Wrong aspect path resolution 🐞 Bug ≡ Correctness
WorkspaceAspectsLoader.resolveInstalledAspectRecursively detects a workspace aspect using ignoreVersion=true but then calls workspace.getComponentPackagePath(), which computes isInWorkspace using an exact-version hasId(). When the dependency graph provides a versioned ID that doesn’t exactly match the workspace bitmap, getRuntimeModulePath may pick the versioned root path instead of the workspace instance, causing aspect resolution/require to fail despite the aspect existing in the workspace.
Agent Prompt
### Issue description
`resolveInstalledAspectRecursively()` treats an aspect as a workspace component when `workspace.hasId(id, { ignoreVersion: true })` is true, but then calls `workspace.getComponentPackagePath(aspectComponent)`. `getComponentPackagePath()` passes `isInWorkspace: this.hasId(component.id)` (exact version) into `dependencyResolver.getRuntimeModulePath()`, so versioned IDs coming from the graph can be treated as “not in workspace” even though an ignore-version match exists.
This can select a different module root (often the versioned/root-node_modules path) instead of the workspace component instance intended by the ignore-version check, which can break requiring aspects (e.g. missing/incorrect dists).
### Issue Context
This inconsistency only triggers when the graph supplies a versioned `aspectComponent.id` that does not exactly match the bitmap version for the same component (but does match without version).
### Fix Focus Areas
- scopes/workspace/workspace/workspace-aspects-loader.ts[892-901]
- scopes/workspace/workspace/workspace.ts[2182-2188]
- scopes/dependencies/dependency-resolver/dependency-resolver.main.runtime.ts[567-571]
### Suggested fix
Pick one of these (prefer the simplest consistent approach):
1) **Make `Workspace.getComponentPackagePath()` consider ignore-version membership**:
- Change `isInWorkspace: this.hasId(component.id)` to `isInWorkspace: this.hasId(component.id, { ignoreVersion: true })` (or equivalent via `getIdIfExist`).
2) **Normalize to the bitmap ID before computing the path** inside `resolveInstalledAspectRecursively()`:
- If `workspace.hasId(aspectComponent.id, { ignoreVersion: true })` is true, obtain the canonical workspace id (e.g. `workspace.getIdIfExist(aspectComponent.id)`), and compute the runtime module path using that canonical id / force `isInWorkspace: true` for the path selection.
Add/adjust a test or log-based reproduction that exercises a workspace aspect whose dependency graph references a different version than the bitmap but same id-without-version.
ⓘ 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 841136a |
…om-manifest # Conflicts: # .bitmap # e2e/harmony/env.e2e.ts # pnpm-lock.yaml # scopes/compilation/compiler/compiler.main.runtime.ts # scopes/workspace/workspace/workspace.ts
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 e9adba6 |
|
Code review by qodo was updated up to the latest commit b0175b9 |
|
Code review by qodo was updated up to the latest commit e9924d4 |
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.