Fix resolving external packages that are ESM only #1346
Conversation
…mlly resolvePathSync. This will fix mupdf
🦋 Changeset detectedLatest commit: 3c540a9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
WalkthroughThe changes introduce a new mechanism for resolving external ECMAScript Module (ESM) packages, specifically addressing issues with the Changes
Possibly related PRs
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: 3
Outside diff range and nitpick comments (7)
.changeset/heavy-jobs-push.md (1)
1-5: LGTM! Consider adding more implementation details.The changeset file correctly describes the purpose of the patch and aligns with the PR objectives. The patch version bump is appropriate for this type of fix.
Consider expanding the description to include more implementation details. For example:
-Fix resolving external packages that are ESM only by falling back to mlly resolvePathSync. This will fix mupdf +Fix resolving external packages that are ESM only by implementing a fallback mechanism using mlly resolvePathSync. This addresses issues with packages like mupdf and improves overall compatibility with ESM-only dependencies.This provides more context about the change and its broader impact.
packages/cli-v3/e2e/fixtures/esm-only-external/trigger.config.ts (1)
6-8: LGTM: Build configuration correctly specifies external dependency.The build configuration properly marks "mupdf" as an external dependency, which aligns with the PR objective of fixing issues with ESM-only external packages.
Consider adding a brief comment explaining why "mupdf" is marked as external, for better maintainability. For example:
build: { + // Mark "mupdf" as external to properly handle this ESM-only package external: ["mupdf"], },packages/cli-v3/e2e/fixtures/esm-only-external/package.json (3)
5-5: LGTM: Package manager is well-specified.The package manager specification is excellent:
- It pins to a specific version of Yarn (4.2.2).
- The inclusion of a SHA256 checksum enhances security and ensures consistency.
Consider adding a comment explaining the importance of the SHA256 checksum for future maintainers. For example:
"packageManager": "[email protected]+sha256.1aa43a5304405be7a7cb9cb5de7b97de9c4e8ddd3273e4dad00d6ae3eb39f0ef", // The SHA256 checksum ensures the integrity and consistency of the Yarn version across different environments
10-12: LGTM: Workspace configuration is appropriate.The workspace configuration is suitable for a monorepo structure:
- It includes all directories under "packages/", which is a common pattern for organizing multiple packages.
Consider adding a README.md file in the root directory explaining the monorepo structure and the purpose of the workspaces. This would help new contributors understand the project organization quickly.
13-16: LGTM: Dependencies are well-specified and align with PR objectives.The dependencies section is well-configured:
- "@trigger.dev/sdk" is pinned to a specific version, ensuring consistency.
- The inclusion of "mupdf" aligns with the PR objectives of handling ESM-only external packages.
Consider pinning the "mupdf" package to a specific version (e.g., "0.3.0" instead of "^0.3.0") to ensure consistent behavior across all environments. This is especially important for test fixtures. You can always update the version manually when needed.
packages/cli-v3/e2e/fixtures/esm-only-external/tsconfig.json (1)
4-10: ECMAScript and module settings look good, with a minor consideration.The configuration uses modern settings appropriate for a Node.js environment with potential browser-like contexts. The use of "NodeNext" for module-related options is suitable for leveraging the latest Node.js features.
Consider enabling
"verbatimModuleSyntax": truefor more precise import/export syntax checking, which can help catch potential issues early in the development process.packages/cli-v3/e2e/fixtures.ts (1)
182-215: LGTM! New test case for ESM-only external package added.The new test case "esm-only-external" has been successfully added and aligns well with the PR objective of handling ESM-only packages. It follows the established structure of existing test cases, which is good for consistency.
Consider adding a comment above this test case to explain its specific purpose of testing ESM-only external packages. This would improve the readability and maintainability of the test suite.
+ // Test case for handling ESM-only external packages { id: "esm-only-external", buildManifestMatcher: { // ... (rest of the test case) }, },
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
packages/cli-v3/e2e/fixtures/esm-only-external/package-lock.jsonis excluded by!**/package-lock.jsonpackages/cli-v3/e2e/fixtures/esm-only-external/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpackages/cli-v3/e2e/fixtures/esm-only-external/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (10)
- .changeset/heavy-jobs-push.md (1 hunks)
- packages/cli-v3/e2e/fixtures.ts (1 hunks)
- packages/cli-v3/e2e/fixtures/esm-only-external/.yarnrc.yml (1 hunks)
- packages/cli-v3/e2e/fixtures/esm-only-external/package.json (1 hunks)
- packages/cli-v3/e2e/fixtures/esm-only-external/pnpm-workspace.yaml (1 hunks)
- packages/cli-v3/e2e/fixtures/esm-only-external/src/trigger/helloWorld.ts (1 hunks)
- packages/cli-v3/e2e/fixtures/esm-only-external/trigger.config.ts (1 hunks)
- packages/cli-v3/e2e/fixtures/esm-only-external/tsconfig.json (1 hunks)
- packages/cli-v3/src/build/externals.ts (4 hunks)
- scripts/publish-prerelease.sh (1 hunks)
Additional comments not posted (18)
packages/cli-v3/e2e/fixtures/esm-only-external/.yarnrc.yml (1)
1-1: LGTM! Configuration aligns with PR objectives.The addition of this
.yarnrc.ymlfile with thenodeLinker: node-modulesconfiguration is appropriate for the PR's goal of improving handling of ESM-only external packages. This setting ensures that Yarn uses the node-modules linker, which is compatible with ESM packages and should work well for the test fixtures in this directory.packages/cli-v3/e2e/fixtures/esm-only-external/pnpm-workspace.yaml (2)
1-1: Good practice: Including reference to related issueThe comment provides a link to a GitHub issue, which is a good practice for maintaining context and traceability.
2-3: LGTM: Proper workspace configurationThe workspace configuration is correctly set up to include all immediate subdirectories of the
packagesdirectory. This is a standard practice for monorepo setups using pnpm.packages/cli-v3/e2e/fixtures/esm-only-external/trigger.config.ts (4)
1-1: LGTM: Import statement is correct.The import statement is properly structured, using a named import for
defineConfigfrom the Trigger.dev SDK v3. This approach is good for maintainability and potential tree-shaking optimizations.
3-9: LGTM: Configuration object structure is correct.The configuration object is properly exported as default and uses the
defineConfigfunction as expected. The structure includes the necessary properties for a Trigger.dev configuration.
4-4: Verify: Is "" the intended project name?The project name is currently set to "", which appears to be a placeholder. Please confirm if this is intentional for testing purposes, or if it should be replaced with an actual project name for production use.
5-5: LGTM: Source directory configuration is appropriate.The
dirsproperty is correctly set to ["./src/trigger"], which follows common conventions for organizing source files. The use of an array allows for easy addition of multiple directories in the future if needed.packages/cli-v3/e2e/fixtures/esm-only-external/src/trigger/helloWorld.ts (2)
1-2: LGTM: Imports are correctly defined.The imports are appropriately structured for the task requirements. The
mupdfimport aligns with the PR objective of handling ESM-only packages.
1-11: Overall implementation successfully meets PR objectives.This file effectively demonstrates the integration of the ESM-only
mupdfpackage within a Trigger.dev task. The simple nature of thehelloWorldtask makes it an excellent test case for verifying the new external package resolution mechanism. The implementation aligns well with the PR objectives and provides a clear example of how ESM-only packages can be utilized in the project.packages/cli-v3/e2e/fixtures/esm-only-external/package.json (3)
1-4: LGTM: Project metadata is well-configured.The project metadata is correctly set up:
- The name "esm-only-external" clearly indicates its purpose.
- It's appropriately marked as private to prevent accidental publication.
- The "type": "module" setting is crucial for enabling ECMAScript module behavior, which aligns with the PR objectives.
17-19: Verify TypeScript version.The TypeScript version specified (5.5.4) appears to be incorrect or not yet released. This could lead to installation issues or unexpected behavior.
Please verify the correct TypeScript version and update accordingly. You can check the latest stable version of TypeScript using the following command:
#!/bin/bash # Description: Check the latest stable version of TypeScript npm view typescript versionAfter running this command, update the TypeScript version in the package.json file to match the latest stable version or the specific version required for your project.
6-9: LGTM: Engine requirements are well-defined, but clarification needed.The engine requirements are well-specified:
- Yarn version matches the packageManager field, ensuring consistency.
- Including specific versions helps maintain a consistent development environment.
Could you clarify the inclusion of pnpm in the engines field? Since Yarn is specified as the packageManager, it's unclear why pnpm is also listed. If pnpm is not actively used in the project, consider removing it to avoid confusion.
To verify pnpm usage in the project, you can run the following script:
Verification successful
LGTM: pnpm is correctly specified as the package manager.
- The project uses pnpm as indicated by the
packageManagerfield and the presence ofpnpm-lock.yaml.- Yarn is listed in the
enginesfield but does not appear to be actively used.Consider removing Yarn from the
enginesfield to avoid confusion.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for pnpm-related files and configurations # Test 1: Check for pnpm-lock.yaml if [ -f pnpm-lock.yaml ]; then echo "pnpm-lock.yaml found, indicating pnpm usage." else echo "pnpm-lock.yaml not found." fi # Test 2: Check for pnpm in scripts grep -n "pnpm" package.json # Test 3: Check for .pnpmrc file if [ -f .pnpmrc ]; then echo ".pnpmrc found, indicating pnpm configuration." else echo ".pnpmrc not found." fiLength of output: 583
packages/cli-v3/e2e/fixtures/esm-only-external/tsconfig.json (2)
1-2: LGTM: File inclusions are appropriate.The file inclusions cover the typical TypeScript project structure and include a specific configuration file, which seems suitable for the project's needs.
1-32: Overall, the tsconfig.json is well-configured for the project's needs.This configuration file is appropriately set up for a modern TypeScript project, particularly one dealing with ESM-only external dependencies. It aligns well with the PR objectives of addressing issues related to ESM-only packages. The use of "NodeNext" for module-related options and the inclusion of DOM libraries suggest a versatile setup capable of handling both Node.js and browser-like environments, which is beneficial for comprehensive testing and development.
While the configuration is solid, the suggested minor enhancements (enabling unused code checks and adding source map support) could further improve the development experience and code quality. These are optional improvements and do not detract from the overall effectiveness of the current configuration.
scripts/publish-prerelease.sh (1)
31-31: Excellent addition for maintaining a clean git stateThe new line
git reset --hard HEADis a valuable safety measure. It ensures that if the user aborts the publish process, the repository is returned to its original state, discarding any changes made by the script up to that point.This addition:
- Improves the script's robustness by preventing unintended changes from lingering in the working directory.
- Aligns with the script's overall approach to maintaining a clean git state, as seen in the initial status check and the final reset after successful execution.
- Provides consistency in behavior, regardless of whether the script completes or is aborted.
packages/cli-v3/src/build/externals.ts (3)
385-391: Check the parameters passed toesmResolveSyncIn your
resolveSyncfunction, when falling back toesmResolveSync, you are passing{ url: resolveDir }as options. Please verify that:
- The
urloption is correctly used inesmResolveSyncand expects a file path or URL string.- The
resolveDirvariable corresponds to a valid URL or directory path required byesmResolveSync.You can test the fallback mechanism with the following script:
#!/bin/bash # Description: Test 'esmResolveSync' fallback with an ESM-only package node -e " import { resolvePathSync } from 'mlly'; const resolved = resolvePathSync('mupdf', { url: 'file://$PWD' }); console.log('Resolved path:', resolved); "Note: Adjust
'file://$PWD'ifesmResolveSyncrequires a different format for theurloption.
16-16: Confirm the correct import ofresolvePathSyncfrommllyYou are importing
resolvePathSyncfrom themllypackage and aliasing it asesmResolveSync. Please verify thatresolvePathSyncis the appropriate function to use for your fallback mechanism and that it behaves as expected in your context.Run the following script to list the available exports from
mllyand confirm thatresolvePathSyncis indeed exported:
69-69: EnsureresolveSynchandles both CommonJS and ESM modules correctlyIn line 69, you are using
resolveSync(external.name, resolveDir). Please confirm that theresolveSyncfunction correctly resolves module paths for both CommonJS and ESM modules, especially in the context of external dependencies.Consider testing the resolution with the following script:
| export const helloWorld = task({ | ||
| id: "helloWorld", | ||
| run: async () => { | ||
| console.log("Hello, World!", { | ||
| metaformat: mupdf.PDFDocument.META_FORMAT, | ||
| }); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
LGTM: Task definition is correct and demonstrates ESM package integration.
The helloWorld task is well-structured and successfully demonstrates the integration of the ESM-only mupdf package. This serves as an excellent test case for the new external package resolution mechanism.
Consider adding error handling to make the task more robust:
export const helloWorld = task({
id: "helloWorld",
run: async () => {
- console.log("Hello, World!", {
- metaformat: mupdf.PDFDocument.META_FORMAT,
- });
+ try {
+ console.log("Hello, World!", {
+ metaformat: mupdf.PDFDocument.META_FORMAT,
+ });
+ } catch (error) {
+ console.error("Error accessing mupdf:", error);
+ throw error; // Re-throw to ensure task failure is properly handled
+ }
},
});This change will provide better visibility into any potential issues with accessing the ESM-only package.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const helloWorld = task({ | |
| id: "helloWorld", | |
| run: async () => { | |
| console.log("Hello, World!", { | |
| metaformat: mupdf.PDFDocument.META_FORMAT, | |
| }); | |
| }, | |
| }); | |
| export const helloWorld = task({ | |
| id: "helloWorld", | |
| run: async () => { | |
| try { | |
| console.log("Hello, World!", { | |
| metaformat: mupdf.PDFDocument.META_FORMAT, | |
| }); | |
| } catch (error) { | |
| console.error("Error accessing mupdf:", error); | |
| throw error; // Re-throw to ensure task failure is properly handled | |
| } | |
| }, | |
| }); |
| "strict": true, | ||
| "alwaysStrict": true, | ||
| "strictPropertyInitialization": false, | ||
| "skipLibCheck": true, | ||
| "forceConsistentCasingInFileNames": true, | ||
| "noUnusedLocals": false, | ||
| "noUnusedParameters": false, | ||
| "noImplicitAny": true, | ||
| "noImplicitReturns": true, | ||
| "noImplicitThis": true, | ||
| "noFallthroughCasesInSwitch": true, | ||
| "resolveJsonModule": true, | ||
| "removeComments": false, | ||
| "esModuleInterop": true, | ||
| "emitDecoratorMetadata": false, | ||
| "experimentalDecorators": false, | ||
| "downlevelIteration": true, | ||
| "isolatedModules": true, | ||
| "noUncheckedIndexedAccess": true, | ||
| "pretty": true | ||
| } |
There was a problem hiding this comment.
Type-checking rules and compiler behaviors are well-configured, with room for minor enhancements.
The strict type-checking rules are appropriately set to catch potential issues early in the development process. The configuration prioritizes correctness and modern JavaScript features, which is commendable.
Consider the following enhancements:
- Enable
"noUnusedLocals": trueand"noUnusedParameters": trueto help identify and remove unused code, improving maintainability. - Add source map configuration, e.g.,
"sourceMap": true, to aid in debugging.
Example:
{
"compilerOptions": {
// ... existing options ...
"noUnusedLocals": true,
"noUnusedParameters": true,
"sourceMap": true
}
}These changes can further improve code quality and developer experience.
| const resolvedPath = nodeResolve.sync(packageName, { | ||
| basedir: args.resolveDir, | ||
| }); | ||
| const resolvedPath = resolveSync(packageName, args.resolveDir); |
There was a problem hiding this comment.
Verify consistent module resolution in createExternalsCollector
At line 147, the resolveSync function is used to resolve packageName. Ensure that this function provides consistent resolution results, and consider any differences in args.resolveDir that might affect the outcome.
If there are discrepancies in resolution paths, it might lead to improperly marked externals. Double-check that args.resolveDir is the correct directory for resolving the package.
commit: |
This fixes an issue where adding an ESM only package to
build.externalwould fail to make the dependency external and would instead bundle the import. For example, mupdf:This is because internally we were only using the resolve package to resolve the import, which is needed to grab the version of the dependency. I've added a fallback to resolve with mlly, which handles will correctly resolve the ESM only package.
Summary by CodeRabbit
New Features
mupdflibrary.Bug Fixes
mupdfpackage to improve functionality.Documentation
Chores