Repository navigation
Conversation
commit: |
webpro
left a comment
There was a problem hiding this comment.
Thanks, Jake! On main, alias targets such as canvas: './empty-module.ts' are reported as unused files, and treating the targets as entries fixes that, also for { browser } targets and package targets like pino/browser. Two things to address: the conditional configs, and the fixture.
| const collectAliases = (node: Node | null | undefined) => { | ||
| if (node?.type === 'ConditionalExpression') { | ||
| collectAliases(node.consequent); | ||
| collectAliases(node.alternate); | ||
| } else if (node?.type === 'LogicalExpression') { | ||
| collectAliases(node.left); | ||
| collectAliases(node.right); | ||
| } else if ( | ||
| node?.type === 'TSAsExpression' || | ||
| node?.type === 'TSSatisfiesExpression' || | ||
| node?.type === 'ParenthesizedExpression' | ||
| ) { | ||
| collectAliases(node.expression); | ||
| } else { | ||
| const aliases = findProperty(node, 'resolveAlias'); | ||
| if (aliases?.type !== 'ObjectExpression') return; | ||
| for (const alias of aliases.properties) { | ||
| if (alias.type === 'Property') collectAliasTargets(alias.value); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Reading through ternaries, && and type assertions to reach turbopack is more than we want to handle in resolveFromAST, as just added to the plugin guide. Could resolveAlias be read from the static turbopack object, next to rules in the ObjectExpression visitor? That removes collectAliases and the Property visitor, and targets of conditional aliases can still be listed as entries in the Knip config.
There was a problem hiding this comment.
Could we keep support for a single ternary at turbopack? An extension-only alias (turbopack: isExtensionBuild ? { resolveAlias: ... } : undefined) otherwise leaves the replacement's exports reported as unused. In a5a117c, the existing ObjectExpression visitor reads static alias objects from both branches; the broader traversal is removed. Regression tests cover both branches.
| import { createOptions } from '../helpers/create-options.ts'; | ||
| import { resolve } from '../helpers/resolve.ts'; | ||
|
|
||
| const cwd = resolve('fixtures/plugins/next-turbopack-aliases'); |
There was a problem hiding this comment.
Could this extend the next-turbopack-loaders fixture instead of adding a workspace? Its next.config.ts already has turbopack.rules and an unrelated control object, so a resolveAlias block with a relative stub, a { browser } stub and a package target such as pino/browser covers what this change handles. The workspace packages and the import from @fixtures/providers don't change the outcome, since the targets become entries regardless of who imports the alias.
There was a problem hiding this comment.
Removed the workspace fixture and extended next-turbopack-loaders with relative, browser, array, and package targets. A small separate fixture covers the conditional config shape. Both retain unused-file controls and pass in normal and production modes.
webpro
left a comment
There was a problem hiding this comment.
Thanks for the revisions! The extended fixture covers each target form, and the single ternary at turbopack for the extension-only alias is fine with us. We might switch to resolveConfig some day to read aliases and loaders from the loaded config, which also covers spreads and variables.
|
🚀 This pull request is included in v6.42.0. See Release 6.42.0 for release notes. Using Knip in a commercial project? Please consider becoming a sponsor. |
Recognize Next.js
turbopack.resolveAliastargets so replacement files, their exports, and package dependencies are treated as used. Supports static string, array, and browser targets, including static objects in either branch of a singleturbopackternary.Extended the existing loader fixture and added a small conditional-config fixture, with unused controls. Regression tests fail without the fix; focused Bun/Node tests, 1,293 smoke tests, build, repo checks, and self-checks pass.