Support scopes packages in externals when found in a transitive dep #1552 #1569
Conversation
🦋 Changeset detectedLatest commit: 6aecfca The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/cli-v3/src/build/externals.tsOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe pull request focuses on improving the handling of scoped packages within the Trigger.dev CLI build process. Specifically, the changes modify the Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/cli-v3/src/build/externals.ts (1)
48-67: Consider enhancing the scoped package handling implementationThe implementation correctly handles scoped packages by creating the necessary directory structure. However, consider these improvements:
- Extract the scope directory logic into a reusable helper function
- Add error handling for the
mkdiroperation- Use a more robust method to extract the scope directory
Consider this refactoring:
+ async function ensureScopeDirectory(packageName: string, destinationPath: string, logger: BuildLogger): Promise<void> { + if (!packageName.startsWith('@')) return; + + const scopeMatch = packageName.match(/^(@[^/]+)/); + if (!scopeMatch) { + logger.debug("[externals] Invalid scoped package name", { packageName }); + return; + } + + const scopeDir = scopeMatch[1]; + const scopePath = join(destinationPath, scopeDir); + + logger.debug("[externals] Creating scope directory", { + scopeDir, + scopePath, + }); + + try { + await mkdir(scopePath, { recursive: true }); + } catch (error) { + logger.debug("[externals] Failed to create scope directory", { + error, + scopePath, + }); + throw error; + } + } async function linkExternal(external: CollectedExternal, resolveDir: string, logger: BuildLogger) { const destinationPath = join(resolveDir, "node_modules"); await mkdir(destinationPath, { recursive: true }); logger.debug("[externals] Make a symbolic link", { fromPath: external.path, destinationPath, external, }); - // For scoped packages, we need to ensure the scope directory exists - if (external.name.startsWith("@")) { - // Get the scope part (e.g., '@huggingface') - const scopeDir = external.name.split("/")[0]; - - if (scopeDir) { - const scopePath = join(destinationPath, scopeDir); - - logger.debug("[externals] Ensure scope directory exists", { - scopeDir, - scopePath, - }); - - await mkdir(scopePath, { recursive: true }); - } else { - logger.debug("[externals] Unable to get the scope directory", { - external, - }); - } - } + await ensureScopeDirectory(external.name, destinationPath, logger);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/kind-plants-tap.md(1 hunks)packages/cli-v3/src/build/externals.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/kind-plants-tap.md
🔇 Additional comments (2)
packages/cli-v3/src/build/externals.ts (2)
48-67: Implementation successfully handles scoped packages
The changes effectively solve the issue of supporting scoped packages in externals. The implementation:
- Correctly identifies scoped packages
- Creates necessary directory structure
- Maintains good logging practices
While there are suggestions for improvements, the current implementation is functional and meets the requirements.
48-67: Verify handling of nested scoped packages
While the implementation handles basic scoped packages, we should verify the handling of nested scoped packages (e.g., @scope/namespace/package) and ensure proper resolution in the build process.
✅ Verification successful
Based on the analysis of the package.json files and dependencies across the codebase, I can now provide a final response:
No issues with nested scoped package handling
The current implementation correctly handles scoped packages. While the codebase uses many scoped packages (like @trigger.dev/*, @opentelemetry/*, etc.), there are no nested scoped packages (packages with format @scope/namespace/package) in use. The current directory creation logic is sufficient for the actual package patterns used in the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for nested scoped packages in the codebase
# Search for import/require statements with nested scoped packages
rg -g '*.{js,ts,jsx,tsx}' -A 2 'from\s+["'\'']@[^/]+/[^/]+/.+["'\'']|require\(["\']@[^/]+/[^/]+/.+["\']\)'
# Search for nested scoped packages in package.json files
fd 'package.json' -x jq -r '.dependencies + .devDependencies | keys[] | select(test("^@[^/]+/[^/]+/.+"))'
Length of output: 116
Script:
#!/bin/bash
# Let's try a different approach to check for nested scoped packages
# Search for import statements with scoped packages to understand current usage patterns
rg -g '*.{js,ts,jsx,tsx}' "from ['\"](@@[^'\"]+)" --no-line-number || true
# Look for dependencies in package.json files
fd "package.json" -x cat {} \; | grep -A 1 "@.*/" || true
# Check the externals handling implementation to understand the current logic
rg -g '*.{ts,js}' -B 2 -A 2 "external.name.startsWith('@')" || true
Length of output: 19115
@trigger.dev/react-hooks
@trigger.dev/build
@trigger.dev/rsc
trigger.dev
@trigger.dev/sdk
@trigger.dev/core
commit: |
ericallam
force-pushed
the
ea-branch-4
branch
from
db0abc6 to
6aecfca
Compare
This fixes #1552. The issue turned out to be marking an external dependency with a scoped name (e.g.
@huggingface/transformers), when it was being imported through another package. This was happening because we create a symlink from the trigger build node_modules dir to the actual location of the dependency (in dev), but that was failing because the@huggingfacedirectory didn't exist.Summary by CodeRabbit
Bug Fixes
New Features
node_modulespath.