Skip to content

fix: don't fail tarball bundling on modules that don't exist if they are optional or caught#7001

Merged
pieh merged 5 commits into
mainfrom
michalpiechowiak/frb-2118-bundling-fails-on-caught-missing-dynamic-import
Mar 24, 2026
Merged

fix: don't fail tarball bundling on modules that don't exist if they are optional or caught#7001
pieh merged 5 commits into
mainfrom
michalpiechowiak/frb-2118-bundling-fails-on-caught-missing-dynamic-import

Conversation

@pieh

@pieh pieh commented Mar 23, 2026

Copy link
Copy Markdown
Contributor

🎉 Thanks for submitting a pull request! 🎉

Summary

Fixes https://linear.app/netlify/issue/FRB-2118/bundling-fails-on-caught-missing-dynamic-import


For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code 🧑‍💻. This ensures
    we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
    something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read the contribution guidelines 📖. This ensures
    your code follows our style guide and passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

@coderabbitai

coderabbitai Bot commented Mar 23, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 67a9f1be-b99c-4fe9-b36a-fa578c29cf72

📥 Commits

Reviewing files that changed from the base of the PR and between a92d700 and 87cadf9.

📒 Files selected for processing (1)
  • packages/edge-bundler/node/bundler.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/edge-bundler/node/bundler.test.ts

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Fixed an issue where the bundler would incorrectly track and report unresolved module dependencies when edge functions attempted to import non-existent modules within try/catch blocks, causing bundling errors even when imports were gracefully handled.
  • Tests

    • Added comprehensive test coverage validating correct bundler behavior when edge functions gracefully handle non-existent module imports during initialization.

Walkthrough

The tarball bundler now avoids including local file:// module paths in getRequiredSourceFiles when those modules have a "Module not found" error. A new fixture edge function attempts a dynamic import of a nonexistent module inside a try/catch and returns a response. A test was added that runs the bundler with tarball output, asserts no dependency-tracking error was logged, verifies manifest.json includes bundling_timing.tarball_ms, checks tarball contents, and executes both the tarball and eszip artifacts; it calls cleanup() after running.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The description includes a reference to the linear issue (FRB-2118) but lacks explanations of motivation and how the fix solves the problem. Add a detailed explanation of the motivation and how the fix prevents bundling failures for optional or caught missing dynamic imports.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: preventing tarball bundling failures for modules that don't exist when they are optional or caught.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch michalpiechowiak/frb-2118-bundling-fails-on-caught-missing-dynamic-import

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Comment on lines +235 to +239
if (module.error?.startsWith('Module not found')) {
// Module graph contains all found imported/required modules, even if they don't actually exist
// This can happen for optional dependencies (dynamic import or require in try/catch).
continue
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this was critical error that prevents code to run - we wouldn't get to this point and would crash hard at Inline Config extraction which does load the bundle and return config export.

So if we get here and we get Module not found errored modules in module graph - this should be case of optional modules (try/catch) OR "virtual" modules (like Next.js Node middleware setup before workaround applied in opennextjs/opennextjs-netlify#3448 )

@pieh pieh marked this pull request as ready for review March 23, 2026 10:05
@pieh pieh requested a review from a team as a code owner March 23, 2026 10:05
@pieh pieh changed the title fix: don't fail bundling on module that don't exist if they are optional or caught fix: don't fail tarball bundling on modules that don't exist if they are optional or caught Mar 23, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/edge-bundler/node/bundler.test.ts`:
- Around line 1268-1272: The test passes extra optional options that reference
non-existent files—remove the configPath and importMapPaths entries from the
bundle(...) call in bundler.test.ts so the test only provides basePath,
featureFlags, and declarations; specifically edit the bundle invocation (the
call to bundle([...], distPath, declarations, {...})) to delete the configPath
and importMapPaths properties and leave the rest unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f723daad-ef3f-4584-aadd-d5f57f454e29

📥 Commits

Reviewing files that changed from the base of the PR and between 238ed96 and a92d700.

📒 Files selected for processing (3)
  • packages/edge-bundler/node/bundler.test.ts
  • packages/edge-bundler/node/formats/tarball.ts
  • packages/edge-bundler/test/fixtures/caught-module-not-found-import/netlify/edge-functions/func1.ts

Comment thread packages/edge-bundler/node/bundler.test.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@pieh pieh force-pushed the michalpiechowiak/frb-2118-bundling-fails-on-caught-missing-dynamic-import branch from 9533e9a to abed9be Compare March 23, 2026 10:30
@pieh pieh enabled auto-merge (squash) March 24, 2026 09:50
@pieh pieh merged commit e3a2934 into main Mar 24, 2026
55 of 56 checks passed
@pieh pieh deleted the michalpiechowiak/frb-2118-bundling-fails-on-caught-missing-dynamic-import branch March 24, 2026 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants