fix: deployment error when using packages using native node modules #36967
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: e4ad437 The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 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 |
Contributor
WalkthroughAdds a changeset bumping @rocket.chat/apps-engine and @rocket.chat/meteor to patch and updates the Deno runtime require-construction to normalize Node built-in specifiers so both bare names (e.g., Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Code
participant Build as buildRequire (Deno runtime)
participant Resolver as Module Resolver
App->>Build: require('crypto') or require('node:crypto')
activate Build
Build->>Build: Normalize specifier (strip "node:" if present)
Build->>Resolver: Is normalized a Node builtin?
alt builtin
Resolver-->>Build: Yes → use "node:<normalized>"
Build-->>App: Return native module
else not builtin
Build->>Resolver: Resolve per external/apps-engine rules
Resolver-->>Build: Module resolved or not found
alt resolved
Build-->>App: Return module
else not allowed
Build-->>App: Throw "Module <module> is not allowed"
end
end
deactivate Build
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36967 +/- ##
===========================================
- Coverage 66.23% 66.21% -0.02%
===========================================
Files 3384 3384
Lines 115027 115027
Branches 21064 21066 +2
===========================================
- Hits 76185 76166 -19
- Misses 36247 36254 +7
- Partials 2595 2607 +12
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.changeset/big-fireants-leave.md (1)
5-5: Clarify the changelog note with the exact behavior.Suggest explicitly mentioning the node: scheme to help readers scanning releases.
-Fixes an issue with deploying Apps that import libraries using native Node modules. +Fix: Apps deployment no longer fails when bundled code imports Node built‑ins using the node: scheme (e.g., node:crypto); both bare (crypto) and node:‑prefixed specifiers are accepted.packages/apps-engine/deno-runtime/handlers/app/construct.ts (2)
29-36: Also support built‑in subpath specifiers (fs/promises, stream/web, util/types).Current check rejects valid Node subpaths like node:fs/promises even though base fs is allowed. Allow subpaths when the base is in the allowlist.
- return (module: string): unknown => { - // Normalize Node built-in specifiers: accept both 'crypto' and 'node:crypto' - const normalized = module.startsWith('node:') ? module.slice(5) : module; + return (module: string): unknown => { + // Normalize Node built-in specifiers: accept both 'crypto' and 'node:crypto' + const normalized = module.startsWith('node:') ? module.slice(5) : module; + const base = normalized.split('/')[0]; // support subpaths like 'fs/promises' and 'stream/web' - if (ALLOWED_NATIVE_MODULES.includes(normalized)) { - return require(`node:${normalized}`); + if (ALLOWED_NATIVE_MODULES.includes(base)) { + // Keep full subpath when present (e.g., 'fs/promises') + return require(`node:${normalized}`); }Optionally add a unit test for node:fs/promises and node:stream/web.
46-47: Improve the error to aid debugging without leaking internals.Include a brief hint for node: vs npm: to reduce support churn.
- throw new Error(`Module ${module} is not allowed`); + throw new Error(`Module ${module} is not allowed. Only selected Node built-ins (node:<name>) and whitelisted npm packages are supported.`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.changeset/big-fireants-leave.md(1 hunks)packages/apps-engine/deno-runtime/handlers/app/construct.ts(1 hunks)
🔇 Additional comments (1)
packages/apps-engine/deno-runtime/handlers/app/construct.ts (1)
29-36: Approve — normalization of Node built-in specifiers is correct.Accepting both bare and 'node:'‑prefixed built‑ins is safe and consistent (repo already contains multiple 'node:' imports).
d-gubert
reviewed
Sep 18, 2025
d-gubert
reviewed
Sep 18, 2025
d-gubert
approved these changes
Sep 18, 2025
Dnouv
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
node:cryptoare allowedand resolved consistently.
Issue(s)
Module node:crypto is not allowed.Steps to test or reproduce
To recreate the issue:
langchainrc-apps deploywill throw the error:Error: Deployment error: Module node:crypto is not allowedFurther comments
RCAI6-7
Summary by CodeRabbit
Bug Fixes
Chores