feat(envs): remove core envs from the manifest and their sources from the workspace#10465
Open
davidfirst wants to merge 188 commits into
Open
feat(envs): remove core envs from the manifest and their sources from the workspace#10465davidfirst wants to merge 188 commits into
davidfirst wants to merge 188 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. Env reload masks missing deps
|
![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 |
… or dep-aspect failures Two load-side fixes for envs whose install layout is imperfect (e.g. the versioned env root misses the env package): 1. a dependency aspect failing to resolve no longer aborts the whole resolveAspects call - only requested aspects still throw. 2. when several versions of an env fall back to the same package dir, keep the def matching the requested id instead of the first one.
…th module-not-found in env suppression - add @bit-no-check to the 4 e2e fixture extensions importing the node env package, aligning them with the sibling fixtures (the package is not installed at the workspace root once the env source is removed). - the legacy-core-env workspace-issue suppression only matched errors quoting the package name; also match the absolute-path form emitted when a resolved path is required.
…undle require() of the ESM-only mdx options package may be hijacked by a stale @babel/register pirates hook (left after the mocha tester runs in-process), which compiles the ESM source as CJS and crashes on the export token. load it with a native import() and pass it to createRspackConfig.
…dx aspect is not loaded the mdx aspect used to be a core aspect, so its import detector for md/mdx files was always registered. now that it's a regular env, docs imports (e.g. a component's .docs.mdx importing another component) silently dropped from the model when no workspace component loads the mdx aspect, breaking preview bundling in capsules. register a regex-based fallback detector in dependency-resolver; the mdx aspect's compile-based detector takes precedence whenever it is loaded.
…in md/mdx import detection imports shown inside fenced code blocks are documentation examples, not real ESM imports - detecting them produced self-import and missing-package issues for docs that demonstrate usage. also anchor imports to line starts, as MDX only treats block-level statements as ESM.
…e .js loader a tester running in-process (e.g. mocha) may leave a @babel/register pirates hook that replaces the .js extension loader and compiles ESM sources as CJS scripts, crashing on the export token (seen when the published react env lazily requires the ESM-only mdx options package during GeneratePreview). capture the pristine loader at bootstrap and retry a failed ESM require with it - node >= 22.12 handles require() of ESM natively.
…nvs-from-workspace # Conflicts: # .bitmap # components/legacy/e2e-helper/excluded-fixtures/extensions/dev-files-env/dev-files-env.extension.ts # components/legacy/e2e-helper/excluded-fixtures/extensions/env-add-dependencies/add-deps-env.extension.ts # components/legacy/e2e-helper/excluded-fixtures/extensions/node-env-dev-dep/node-env.extension.ts # pnpm-lock.yaml # scopes/react/ui/docs/apply-providers/apply-providers.tsx # scripts/e2e-test-timings.json
davidfirst
changed the title
…nvs-from-workspace # Conflicts: # .bitmap
…d by getEnvId slot match
…om-manifest # Conflicts: # .bitmap # pnpm-lock.yaml # scopes/mdx/mdx/mdx.detector.spec.ts # scopes/mdx/mdx/mdx.detector.ts
…v source removed)
|
Code review by qodo was updated up to the latest commit 86ede67 |
…y not-yet-compiled reloads
…om-manifest # Conflicts: # .bitmap
Comment on lines
+249
to
+255
| private getFallbackDefaultEnv(): Environment { | ||
| if (!this.fallbackDefaultEnv) { | ||
| this.fallbackDefaultEnv = { | ||
| name: 'node', | ||
| getCompiler: () => getFallbackTypescriptCompiler(), | ||
| __getDescriptor: async () => ({ type: 'node' }), | ||
| }; |
There was a problem hiding this comment.
1. Fallback env mislabels env 🐞 Bug ≡ Correctness
EnvsMain.getFallbackDefaultEnv() hardcodes name: 'node' and descriptor { type: 'node' }, but it
is used as the env instance when a legacy core env isn't loadable, causing non-node envs (e.g.
react/mdx) to be reported/typed as node. This can mislead bit show/env descriptor logic and any
behavior branching on env name/type while the real env is missing.
Agent Prompt
### Issue description
`EnvsMain.getFallbackDefaultEnv()` returns an Environment object that identifies itself as `name: 'node'` and `__getDescriptor(): { type: 'node' }`. This fallback is used when legacy core envs are not installed/loaded, which can cause Bit to report a missing React/MDX/etc env as if it were a Node env.
### Issue Context
This fallback is used as a safety net for default-env bootstrap and also as a stand-in env definition for missing legacy-core envs, while env descriptors (and user-facing reporting) use `envDef.name` and `envDef.env.__getDescriptor().type`.
### Fix Focus Areas
- scopes/envs/envs/environments.main.runtime.ts[230-258]
- scopes/envs/envs/environments.main.runtime.ts[604-617]
- scopes/envs/envs/environments.main.runtime.ts[761-775]
### Suggested fix
- Change the fallback env identity to be explicitly "fallback" (e.g. `name: 'fallback-env'`, `__getDescriptor: async () => ({ type: 'fallback' })`).
- Keep the compiler fallback if needed, but avoid claiming the env is `node` when it is only a temporary shim for a different env id.
- (Optional) If you want to preserve the requested env id in reporting, consider computing name/type from the env-id at the call site (e.g. in `getEnv()` when wrapping `new EnvDefinition(id, ...)`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+749
to
+760
| try { | ||
| await runtime.provider(undefined, undefined, undefined, this.harmony); | ||
| } catch (err: any) { | ||
| const notRequirableYet = | ||
| err.code === 'MODULE_NOT_FOUND' || | ||
| err.code === 'ERR_MODULE_NOT_FOUND' || | ||
| err.code === 'ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING' || | ||
| err.message?.includes('Cannot find module'); | ||
| if (!notRequirableYet) throw err; | ||
| this.logger.consoleWarning( | ||
| `unable to reload the env ${id.toString()}, it will be loaded once compiled. error: ${err.message}` | ||
| ); |
There was a problem hiding this comment.
2. Env reload masks missing deps 🐞 Bug ☼ Reliability
InstallMain.reloadOneAspectsGroup() treats MODULE_NOT_FOUND/ERR_MODULE_NOT_FOUND (and any message containing "Cannot find module") as a transient "not compiled yet" state and only logs a warning, so genuine missing dependency errors in an env/plugin can be swallowed. This can make `bit install` appear successful while leaving the env/plugin broken until a later, harder-to-diagnose failure.
Agent Prompt
### Issue description
During install, `reloadOneAspectsGroup()` runs each aspect/plugin runtime provider and catches errors. The suppression predicate currently treats generic module-resolution failures (`MODULE_NOT_FOUND`, `ERR_MODULE_NOT_FOUND`, and any message containing `Cannot find module`) as "not requirable yet" and continues.
This is too broad: these errors also represent real missing dependencies (not just missing compiled outputs), contradicting the nearby comment that genuine errors must still fail the install.
### Issue Context
- The code path is part of install-time reload of envs/aspects.
- The warning message says "it will be loaded once compiled", which is inaccurate for real missing dependency scenarios.
- Elsewhere, the system generally treats aspect load failures as actionable and suggests running `bit install`.
### Fix Focus Areas
- scopes/workspace/install/install.main.runtime.ts[713-764]
- scopes/harmony/aspect-loader/constants.ts[3-15]
### Suggested fix
- Narrow the tolerated error set to only cases that are truly transient for this flow (e.g. `ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING`, or missing *the env’s own dist entrypoint*).
- For `MODULE_NOT_FOUND` / `Cannot find module`, only suppress if you can prove the missing path is within the env’s own uncompiled output (e.g. error refers to `<localPath>/dist/...`). Otherwise rethrow so install fails loudly.
- Update the warning text to avoid implying compilation will fix all module-not-found errors (or include guidance like "run bit install" when appropriate).
ⓘ 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 50d29a5 |
Comment on lines
+725
to
+729
| private getDistMain(component: Component, localPath: string): string | undefined { | ||
| const mainFile = component.state._consumer.mainFile; | ||
| if (!mainFile) return undefined; | ||
| const distMain = join(localPath, 'dist', mainFile.replace(/\.(ts|tsx|mts|cts|jsx)$/, '.js')); | ||
| return fs.pathExistsSync(distMain) ? distMain : undefined; |
There was a problem hiding this comment.
1. Hardcoded dist dirname 🐞 Bug ⚙ Maintainability
WorkspaceAspectsLoader.getDistMain() hardcodes the compiled output directory name as 'dist', duplicating the default dist-dir policy used elsewhere (DEFAULT_DIST_DIRNAME). If the default dist dir ever changes, this fallback will diverge and can break aspect loading via the stale-"main" workaround.
Agent Prompt
### Issue description
`WorkspaceAspectsLoader.getDistMain()` constructs a fallback path using a hardcoded `'dist'` directory name. This duplicates dist-directory policy that is already centralized via `DEFAULT_DIST_DIRNAME` elsewhere in the codebase.
### Issue Context
The fallback is used when requiring an aspect from `node_modules` fails due to a stale TypeScript `main` entry. Other loaders (e.g. `AspectLoaderMain`) already use `DEFAULT_DIST_DIRNAME` when falling back to dists, so this path should stay consistent.
### Fix Focus Areas
- scopes/workspace/workspace/workspace-aspects-loader.ts[725-729]
ⓘ 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 3ebce3b |
…om-manifest # Conflicts: # .bitmap # pnpm-lock.yaml
Comment on lines
+871
to
+875
| // guard against circular dependencies in the graph. the set acts as the active call stack - | ||
| // ids are removed when unwinding (finally below), so it never misclassifies reconverging | ||
| // (diamond) paths as cycles. | ||
| if (visiting.has(aspectStringId)) return undefined; | ||
| visiting.add(aspectStringId); |
There was a problem hiding this comment.
1. Cycle resolution fails silently 🐞 Bug ◔ Observability
WorkspaceAspectsLoader.resolveInstalledAspectRecursively() returns undefined immediately when it detects a cycle via visiting.has(aspectStringId), but it only emits warnings/issues when lastError is set; cycle-triggered failures therefore produce no console warning and no reportLoadFailure, making missing installed-aspect resolution hard to diagnose.
Agent Prompt
### Issue description
`resolveInstalledAspectRecursively()` detects a cycle and returns `undefined` early. Because the warning + `reportLoadFailure(...)` path is only executed when `lastError` is populated, cycle-based failures are silently swallowed.
### Issue Context
- The function uses `visiting` as an active recursion stack.
- On cycle detection it returns before:
- setting `this.resolvedInstalledAspects` for the failed id
- producing a console warning
- calling `reportLoadFailure`
### Fix Focus Areas
- scopes/workspace/workspace/workspace-aspects-loader.ts[859-933]
### Suggested fix
- When `visiting.has(aspectStringId)` is true:
- emit a warning (and/or `reportLoadFailure`) that explicitly mentions a circular dependency during `resolve-installed-aspect`.
- optionally cache the failure (`this.resolvedInstalledAspects.set(aspectStringId, null)`) to prevent repeated attempts.
- if `opts.throwOnError` is true (requested aspect), throw a cycle-specific error that includes at least the current id; ideally include the cycle path if available.
ⓘ 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 3d8f969 |
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.