Conversation
| @@ -1,21 +1,226 @@ | |||
| import { workspace } from 'vscode'; | |||
| import * as path from 'path'; | |||
| import { createRequire } from 'module'; | |||
There was a problem hiding this comment.
I'm not sure we could have it for web-worker version, wondering if we could do lazy import of createRequire?
|
|
||
| async function checkBuildFile(folderFsPath: string): Promise<boolean> { | ||
| for (const name of EMBER_CLI_BUILD_FILES) { | ||
| const uri = Uri.file(path.join(folderFsPath, name)); |
There was a problem hiding this comment.
I'm not sure we could use path.join (for example, for virtual modules)
https://github.com/microsoft/vscode-uri#usage-util wondering if Utils.joinPath or Utils.resolvePath(URI, paths) help us here
| return false; | ||
| } | ||
|
|
||
| async function isMonorepoRoot(dir: string): Promise<boolean> { |
There was a problem hiding this comment.
let's give it a more explicit name: isPNPMMonorepoRoot (to not confuse ourselfs)
lifeart
left a comment
lifeart
left a comment
There was a problem hiding this comment.
A few additional observations beyond the earlier thread. Nothing blocking; mainly correctness/UX polish.
One meta point: this extension declares "browser": "./dist/web/client/browserClientMain" in package.json, so web-worker compatibility (raised earlier on createRequire) is a live concern, not hypothetical.
Also — no tests were added. The tiered detection has several non-obvious branches (resolver path, walk cap, monorepo-root detection, the step-counter logic below); a small table-driven test over a tmp fixture tree would make this much safer to evolve.
| // the top of the workspace; going one level above handles layouts that | ||
| // nest the repo inside another tooling directory. | ||
| if (await isMonorepoRoot(dir)) { | ||
| stepsAfterMonorepoRoot = 1; |
There was a problem hiding this comment.
Off-by-one vs the comment on line 194. The comment says "allow one more ancestor after we hit one," but setting stepsAfterMonorepoRoot = 1 actually causes markers to be checked at the monorepo root and two ancestors above it:
- iter N (monorepo root): check markers, set
steps = 1, go to parent - iter N+1: check markers,
steps === 1→ decrement to0, go to parent - iter N+2: check markers,
steps === 0→ break
So we visit three directories starting at the monorepo root, not two. Either set stepsAfterMonorepoRoot = 0 here, or update the comment to match ("allow two more ancestors"). Worth a test either way.
| const folders: ReadonlyArray<WorkspaceFolder> = | ||
| workspace.workspaceFolders ?? []; | ||
| if (folders.length === 0) { | ||
| return false; |
There was a problem hiding this comment.
When detection returns false across all folders, the user gets no signal — they just see the extension silently not activate, and forceEnable is the only recourse. Consider an output-channel log line ("ELS: no Ember markers found in ; set els.detection.forceEnable to override"). Saves a round-trip when someone hits a new exotic layout.
| if (!deps || typeof deps !== 'object') continue; | ||
| const depNames = Object.keys(deps as Record<string, unknown>); | ||
| for (const marker of EMBER_MARKER_PACKAGES) { | ||
| if (depNames.indexOf(marker) !== -1) { |
There was a problem hiding this comment.
Minor: marker in deps (or Object.prototype.hasOwnProperty.call(deps, marker)) would skip the Object.keys allocation and the O(N) scan per marker — looks up directly in the dep map. depNames.includes(marker) is also clearer than .indexOf(...) !== -1 if you prefer to keep the keys array.
| "workspaceContains:ember-cli-build.js", | ||
| "workspaceContains:**/ember-cli-build.js", | ||
| "workspaceContains:**/ember-cli-build.cjs", | ||
| "workspaceContains:**/node_modules/ember-source/package.json", |
There was a problem hiding this comment.
Two concerns on the activation globs here:
- Broader match than before.
workspaceContains:**/ember-cli-build.jsmatches nested copies too (e.g. vendored build files inside unrelated packages), which will activate ELS in non-Ember roots and can slow activation on large monorepos. Previously this was root-only (workspaceContains:ember-cli-build.js). Consider keeping the root-only pattern and letting the richer detection logic handle deeper cases. - Marker drift.
EMBER_MARKER_PACKAGESin workspace-utils.ts lists six packages (incl.glimmer-lite-core,@glimmerx/core,ember-template-imports, etc.), but activation only triggers onember-source. If a user opens a repo that only depends on one of the other markers and the extension never activates for aonLanguage:*reason, detection never runs. Either expand theworkspaceContainslist to match, or document that non-ember-sourceprojects rely on a language activation.
| "devDependencies": { | ||
| "@types/mocha": "^2.2.33", | ||
| "@types/node": "^6.0.52", | ||
| "@types/node": "^14.18.63", |
There was a problem hiding this comment.
Unrelated to the detection feature — an eight-major-version bump of @types/node (6 → 14) is likely to surface type-surface changes elsewhere. Worth splitting into its own PR, or at minimum confirming tsc --noEmit is clean across the repo (and the browser/web build still type-checks, since that bundle swaps Node builtins for shims).
There was a problem hiding this comment.
The big bump was needed to properly type createRequire, technically createRequire needs 12.x.x but 14.x.x matches the major version of node shipped in vscode 1.60.0, which is the minimum engine version defined in package.json.
I've built with tsc but I'll do a full check for browser/web build.
2 participants
This adds more aggressive ember project detection methods.
I often open a pnpm monorepo package as my workspace which means
workspace.findFileswill not match files because the node_modules symlinks point outside the workspace root.This uses 5 methods to detect an ember project starting with the cheapest
Tested on my project and it works.