floydspace/serverless-esbuild

Non-root dependency of external package gets filtered out by bundleDeps AST check

flipscholtz opened this issue · 3 comments

flipscholtz commented

Describe the bug
When a child of an 'external' package appears in the root of the packager output tree such as yarn list --json, it gets filtered out here after getting the list of bundle deps via acorn, because children of external dependencies aren't directly imported from the bundle.js AST.

The comment here claims the filtering is done 'only for root level dependencies' but the if statement doesn't check details.isRootDep .
I think the packager may list a package at the top-level of the tree even if it's not really a root dependency but rather referenced from other dependencies further down.

I tested and adding an isRootDep check to this line fixes it for me:

// only for root level dependencies
if (details.isRootDep && filter && !filter.includes(depName)) {
    \-----------------/
     adding this

but I am unsure of potential side-effects elsewhere because it breaks one of the tests.

To Reproduce
I am working on a minimal example, a bit tricky because it only seems to happen in complex cases like monorepos with lots of inter-dependencies where there's multiple references to a single package.

Expected behavior
If a package is marked as external and its child appears at the top-level of 'yarn list', the child should not be filtered out of node_modules even though it's not directly imported from the bundle.js AST.

Screenshots or Logs
N/A

Versions (please complete the following information):

  • OS: MacOS Ventura 1.13.0
  • Serverless Framework Version: 3.33.0
  • Plugin Version: 1.46.0

Additional context
Add any other context about the problem here.

johnsonps08 commented

@flipscholtz wondering if you ever found a resolution to this issue?

flipscholtz commented

@flipscholtz wondering if you ever found a resolution to this issue?

@johnsonps08 I just pushed a PR, I had this fix on my fork and it worked for me.
#542

flipscholtz commented

@flipscholtz wondering if you ever found a resolution to this issue?

@johnsonps08 I just pushed a PR, I had this fix on my fork and it worked for me. #542

I think the change breaks some of the tests though and I didn't have time to properly investigate yet. I only know it works for my particular set of dependencies.