Add Code Link npm strategy modes#643
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
the
Auto submit to Marketplace on merge
There was a problem hiding this comment.
Pull request overview
Adds support for “unsupported npm” handling in Code Link by introducing explicit npm strategy modes, enabling either ATA-based type acquisition or a package-manager–driven workflow that updates package.json and defers installs to the user’s package manager.
Changes:
- Introduces
NpmStrategy(none/acquire-types/package-manager) with CLI flag parsing and project-level resolution (package.json + lockfile detection). - Adds a new CLI↔Plugin message pair to request dependency versions from the plugin, used to keep
package.jsondependencies current in package-manager mode. - Migrates legacy Code Link
package.jsonmetadata fields intocodeLink.*and updatesframer-plugindependency/version pins.
Reviewed changes
Copilot reviewed 17 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Updates lockfile entries for framer-plugin@3.11.0-alpha.14. |
| plugins/code-link/src/messages.ts | Handles new request-dependency-versions message and replies with dependency-versions. |
| plugins/code-link/src/api.ts | Adds fetchDependencyVersions() using the Framer plugin API. |
| plugins/code-link/package.json | Bumps framer-plugin dependency to 3.11.0-alpha.14. |
| packages/code-link-shared/src/types.ts | Adds DependencyVersions and new CLI↔Plugin message types. |
| packages/code-link-shared/src/index.ts | Re-exports DependencyVersions. |
| packages/code-link-cli/src/utils/project.ts | Adds readAndMigratePackageJson() and migrates Code Link metadata under package.json.codeLink. |
| packages/code-link-cli/src/utils/project.test.ts | Updates existing expectations to codeLink.* and adds migration tests. |
| packages/code-link-cli/src/types.ts | Replaces allowUnsupportedNpm with npmStrategy and adds NpmStrategy type. |
| packages/code-link-cli/src/index.ts | Adds --unsupported-npm [mode] parsing and wires npmStrategy into config. |
| packages/code-link-cli/src/helpers/npm-strategy.ts | New helper to resolve strategy from CLI flag / package.json / lockfiles. |
| packages/code-link-cli/src/helpers/installer.ts | Implements package-manager mode (dependency collection + package.json refresh) and strategy-aware ATA behavior. |
| packages/code-link-cli/src/controller.ts | Resolves npm strategy at runtime, adds dependency version request/response flow. |
| packages/code-link-cli/src/controller.rename.test.ts | Updates config shape for new npmStrategy. |
| packages/code-link-cli/src/controller.once.test.ts | Updates config shape for new npmStrategy. |
| packages/code-link-cli/src/controller.integration.test.ts | Updates config shape and adjusts sync-status message field typing in test parsing. |
| packages/code-link-cli/src/controller.apply.test.ts | Updates config shape for new npmStrategy. |
| packages/code-link-cli/package.json | Bumps CLI version and adds stableVersion metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function parseUnsupportedNpmMode(mode: string | undefined): NpmStrategy { | ||
| if (mode === undefined) { | ||
| return "acquire-types" | ||
| } | ||
|
|
||
| if (mode === "acquire-types" || mode === "package-manager") { | ||
| return mode | ||
| } | ||
|
|
||
| throw new InvalidArgumentError("unsupported npm mode must be 'acquire-types' or 'package-manager'") | ||
| } |
| export async function resolveNpmStrategy(config: Config, projectDir: string): Promise<NpmStrategy> { | ||
| if (config.npmStrategy) { | ||
| debug(`Using npm strategy from CLI flag: ${config.npmStrategy}`) | ||
| return config.npmStrategy | ||
| } | ||
|
|
||
| const packageJsonStrategy = await readPackageJsonStrategy(projectDir) | ||
| if (packageJsonStrategy) { | ||
| debug(`Using npm strategy from package.json ${CONFIG_FIELD}: ${packageJsonStrategy}`) | ||
| return packageJsonStrategy | ||
| } | ||
|
|
||
| const detectedLockfile = await detectLockfile(projectDir) | ||
| if (detectedLockfile) { | ||
| debug(`Using npm strategy package-manager from ${detectedLockfile}`) | ||
| return "package-manager" | ||
| } | ||
|
|
||
| debug("Using default npm strategy: none") | ||
| return "none" | ||
| } |
| @@ -211,9 +227,10 @@ | |||
| const coreImports = await this.buildPinnedImports(CORE_LIBRARIES) | |||
|
|
|||
| // After pins are resolved, also include package.json deps | |||
| const packageJsonDeps = this.allowUnsupportedNpm | |||
| ? Object.keys(this.pinnedTypeVersions).filter(name => !SUPPORTED_PACKAGES.has(name)) | |||
| : [] | |||
| const packageJsonDeps = | |||
| this.npmStrategy === "acquire-types" | |||
| ? Object.keys(this.pinnedTypeVersions).filter(name => !SUPPORTED_PACKAGES.has(name)) | |||
| : [] | |||
|
|
|||
| const imports = [...coreImports, ...(await this.buildPinnedImports(packageJsonDeps))].join("\n") | |||
| await this.ata(imports) | |||
| @@ -226,13 +243,19 @@ | |||
| private async processImports(fileName: string, content: string): Promise<void> { | |||
| const allImports = extractImports(content).filter(i => i.type === "npm") | |||
|
|
|||
| if (allImports.length === 0) return | |||
|
|
|||
| if (this.npmStrategy === "package-manager") { | |||
| await this.enqueuePackageJsonRefresh(allImports.map(imp => getBasePackageName(imp.name))) | |||
| return | |||
| } | |||
Co-authored-by: Cursor <cursoragent@cursor.com>
iamakulov
left a comment
iamakulov
left a comment
There was a problem hiding this comment.
Sorry, this is a big review, but a big chunk of it is about code complexity or DX. Feel free to skip those if it blocks shipping!
Some extra notes from QA/Slack:
- [dx] the version ranges that we set in
package.jsonshould be pinned, not^. we pin your dependency versions in framer, and thepackage.jsonshould reflect that - [bug] with this project, dependency syncing simply doesn’t happen. (reproduced with
npx framer-code-link@alpha --unsupported-npm=package-manager A8ekr3Wn.) files sync, but deps aren’t added intopackage.json. haven’t debugged why.
| private async addPackageNamesFromDirectory(directory: string, packageNames: Set<string>): Promise<void> { | ||
| let entries: Dirent[] | ||
| try { | ||
| entries = await fs.readdir(directory, { withFileTypes: true }) |
There was a problem hiding this comment.
[potential bug] Does this need recursive: true? (Code files can be in subdirectories.)
|
|
||
| try { | ||
| const content = await fs.readFile(entryPath, "utf-8") | ||
| for (const imported of extractImports(content).filter(i => i.type === "npm")) { |
There was a problem hiding this comment.
[simplicity] Up to you whether to drive-by this but I noticed we only ever do extractImports().filter(i => i.type === "npm"). We can drop URL imports and simplify.
There was a problem hiding this comment.
[bug (pre-existing)] We can also switch from fragile regexes (eg they don’t work with imports like
import Foo, {
useFoo,
} from "foo"| /** | ||
| * Fire-and-forget processing of a component file to fetch missing types. | ||
| * JSON files are ignored. | ||
| */ |
There was a problem hiding this comment.
| /** | |
| * Fire-and-forget processing of a component file to fetch missing types or add `package.json` dependencies. | |
| * JSON files are ignored. | |
| */ |
| private async processImports(fileName: string, content: string): Promise<void> { | ||
| const allImports = extractImports(content).filter(i => i.type === "npm") | ||
|
|
||
| if (allImports.length === 0) return | ||
|
|
||
| if (this.npmStrategy === "package-manager") { | ||
| await this.enqueuePackageJsonRefresh(allImports.map(imp => getBasePackageName(imp.name))) | ||
| return | ||
| } |
There was a problem hiding this comment.
[simplicity] I think this would read much simpler if we rearrange this into:
private async processImports(fileName: string, content: string): Promise<void> {
const allImports = extractImports(content).filter(i => i.type === "npm")
if (allImports.length === 0) return
if (this.npmStrategy === "package-manager") {
await this.enqueuePackageJsonRefresh(allImports.map(imp => getBasePackageName(imp.name)))
} else if (this.npmStrategy === "acquire-types") {
await this.acquireTypes(allImports)
} else {
const supportedImports = allImports.filter(i => this.isSupportedPackage(i.name))
const unsupportedImports = allImports.filter(i => this.isSupportedPackage(i.name))
if (unsupportedImports.length > 0) {
debug(`Skipping unsupported packages: ${unsupported.join(", ")} (use --unsupported-npm to enable)`)
}
await acquireTypes(supportedImports)
}WDYT?
| private async enqueuePackageJsonRefresh(packageNames: string[]): Promise<void> { | ||
| const missingPackageNames = packageNames | ||
| .map(name => getBasePackageName(name)) |
There was a problem hiding this comment.
[simplicity] This is weird; two parts of this code tell different stories:
- The param name (
packageNames) tells that the function accepts sanitized package names (foo,@bar/baz) - But
.map(name => getBasePackageName(name))tells that the function accepts raw import paths that need to be sanitized into package names (@bar/baz/dist/index.js)
Looking at the usage, it seems the first story is true. Could we/should we simplify so that the story is consistent?
| private async enqueuePackageJsonRefresh(packageNames: string[]): Promise<void> { | |
| const missingPackageNames = packageNames | |
| .map(name => getBasePackageName(name)) | |
| private async enqueuePackageJsonRefresh(packageNames: string[]): Promise<void> { | |
| const missingPackageNames = packageNames |
| function parseUnsupportedNpmMode(mode: string | undefined): NpmStrategy { | ||
| if (mode === undefined) { | ||
| return "acquire-types" | ||
| } | ||
|
|
||
| if (mode === "acquire-types" || mode === "package-manager") { | ||
| return mode | ||
| } | ||
|
|
||
| throw new InvalidArgumentError("unsupported npm mode must be 'acquire-types' or 'package-manager'") | ||
| } |
There was a problem hiding this comment.
[bug] Don’t we also support "none"? (See npm-strategy.ts)
| .option("--unsupported-npm", "Allow type acquisition for unsupported npm packages") | ||
| .option( | ||
| "--unsupported-npm [mode]", | ||
| "Handle unsupported npm packages (acquire-types or package-manager)", |
There was a problem hiding this comment.
[dx]
| "Handle unsupported npm packages (acquire-types or package-manager)", | |
| "Handle unsupported npm packages (none or acquire-types or package-manager)", |
| .option("--dangerously-auto-delete", "Automatically delete remote files without confirmation") | ||
| .option("--unsupported-npm", "Allow type acquisition for unsupported npm packages") | ||
| .option( | ||
| "--unsupported-npm [mode]", |
There was a problem hiding this comment.
[bug] Devin says (and I confirmed this is true):
--unsupported-npm without argument silently has no effect due to Commander optional-arg semantics
When --unsupported-npm is used without a value argument (the backward-compatible form), Commander does not call the parseUnsupportedNpmMode parse function. Instead, Commander sets the option value to true (boolean) because presetArg is undefined and the option uses [mode] (optional argument). This is confirmed by Commander's source code at node_modules/commander/lib/command.js:692-716 where val is null for optional options without an argument, and since presetArg is undefined, the parse function is skipped and val is set to true.
The boolean true flows into config.npmStrategy, passes resolveNpmStrategy's truthy check (npm-strategy.ts:11), and reaches the Installer where it doesn't match "acquire-types" or "package-manager", silently defaulting to "none" behavior. Users upgrading from the old boolean --unsupported-npm flag would find type acquisition for unsupported packages no longer works.
| const filteredContent = | ||
| this.npmStrategy === "acquire-types" ? content : await this.buildFilteredImports(imports) |
There was a problem hiding this comment.
[bug (pre-existing)] This is a pre-existing bug, so just thought to note: I think we should (not necessarily in this PR) pin versions with this.npmStrategy === "acquire-types" too. Otherwise, a project might use foo@4.0.0, but we might accidentally acquire types for foo@5.0.0. We didn’t have an ability to pin/resolve versions in the past, but we do have it now.
(As a side note, do we know if anyone uses this.npmStrategy === "acquire-types"? Do we need to keep it around now that this.npmStrategy === "package-manager" is here? Maybe the latter one is what everyone would prefer?)
| return [...packageNames] | ||
| } | ||
|
|
||
| private async addPackageNamesFromDirectory(directory: string, packageNames: Set<string>): Promise<void> { |
There was a problem hiding this comment.
[simplicity] Had to pause for a moment to figure out where exactly this function adds package names. Could we / should we make it pure, as in, make it return a set rather than amend an existing set?
Then, above, we can simply do:
diff --git a/packages/code-link-cli/src/helpers/installer.ts b/packages/code-link-cli/src/helpers/installer.ts
index 4dd88e0591..b874a5db05 100644
--- a/packages/code-link-cli/src/helpers/installer.ts
+++ b/packages/code-link-cli/src/helpers/installer.ts
@@ -295,9 +295,10 @@
}
private async collectPackageManagerPackageNames(): Promise<string[]> {
- const packageNames = new Set([...CORE_LIBRARIES, ...PACKAGE_MANAGER_DEV_DEPENDENCIES])
- await this.addPackageNamesFromDirectory(path.join(this.projectDir, "files"), packageNames)
- return [...packageNames]
+ const defaultPackageNames = new Set([...CORE_LIBRARIES, ...PACKAGE_MANAGER_DEV_DEPENDENCIES])
+ const packageNamesFromDir = await this.getPackageNamesFromDirectory(path.join(this.projectDir, "files"))
+
+ return [...defaultPackageNames.union(packageNamesFromDir)]
}
private async addPackageNamesFromDirectory(directory: string, packageNames: Set<string>): Promise<void> {(set.union() is supported as of Node.js 22.)
| let changed = false | ||
| for (const packageName of uniquePackageNames) { | ||
| const version = versions[packageName] ?? this.pinnedTypeVersions[packageName] | ||
| const version = (versions[packageName] ?? this.pinnedTypeVersions[packageName])?.replace(/^\^/, "") |
There was a problem hiding this comment.
[simplicity] why do we have to strip the ^ here? this feels like the wrong place – we have code that adds a ^ somewhere, and we’re undoing that code’s work with more code here. we can just delete the original code, I think?
(is it unstable_getDependencyVersion that returns a version with a ^? then I’d def kill the ^ there. i’d expect the getDependencyVersion method to return a version, not a version range.)
There was a problem hiding this comment.
The ^ is persisted in the actual dependencies.json records for all Framer projects sadly, its something that is technically incorrect but was rushed through years ago.
Would you expect it here or in the Plugin API functions? fixing the source should happen holistically with the other npm fixes IMO, has to be monkey patched somewhere in this process for now.
There was a problem hiding this comment.
Maybe doing it in the Plugin API/FramerStudio at least makes more sense so it can later have the knowledge if the project is fixed/migrated once that happens
Description
Adds Code Link npm strategy handling so unsupported npm can run in
acquire-typesmode orpackage-managermode. Package-manager mode readscodeLinkNpmStrategy/lockfiles, skips ATA writes intonode_modules, and asks the plugin for Framer dependency versions to keeppackage.jsondependencies current.Changelog
--unsupported-npm=acquire-typesand--unsupported-npm=package-manager.codeLink.npmStrategyconfig and lockfile auto-detection for package-manager mode.Testing
acquire-typesmodeplugins/code-link.--unsupported-npm=acquire-typesand verify unsupported package types are acquired.package-managermodeplugins/code-link.--unsupported-npm=package-manager, verifypackage.jsonupdates, then run your package manager.