Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JSii silently forgets to include bundledDependencies in a monorepo #4132

Closed
TimothyJones opened this issue Jun 5, 2023 · 6 comments
Closed
Labels
bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.

Comments

@TimothyJones
Copy link
Contributor

Describe the bug

When using an npm workspaces, with the following structure:

packages/
├─ non-jsii-package/
│  ├─ package.json    // version 0.1.2
├─ jsii-package/
│  ├─ package.json
package.json

where jsii-package/package.json includes:

{
...
   "dependencies": {
    "non-jsii-package": "0.1.2"
  },
  "bundledDependencies": [
    "non-jsii-package"
  ],

  "jsii": {
    "targets": {
      "java": {
        "package": "jsii_package",
        "maven": {
          "groupId": "somegroup",
          "artifactId": "jsii-package"
        }
      }
    }
}

JSii will not include the package in (at least) the Java bundle.

However, if you copy jsii-package/ outside the monorepo and run npm install && npx jsii-pacmak then it will bundle correctly.

Expected Behavior

I expect the generated Java package to include the bundled dependency.

Or, if jsii can't find the dependency at the time it is trying to bundle, I would expect it to fail with an error.

Current Behavior

The generated java package does not include the bundle, but the packaging otherwise appears successful.

This results in broken packages being published, which would result in:

software.amazon.jsii.JsiiError: Cannot find module 'non-jsii-package'

Reproduction Steps

Follow the steps above

Possible Solution

I haven't looked, but I guess that it's expecting the module to be in node_modules/non-jsii-module and failing to find it. If this is the case, I think:

  1. Npm's module resolution should be asked where the module is, instead of looking at the file tree
  2. If the module can't be found it would be an improvement to fail with an error, which would prevent broken packages being published.

Additional Information/Context

If you'd like to see this on a real example, the jsii package @contract-case/case-example-mock-types suffers from this problem (with the non-jsii dependency @contract-case/case-entities-internal).

To reproduce:

git clone git@github.com:case-contract-testing/contract-case.git
cd contract-case
npm install
npm run package         # This will build / package all dependencies of all packages
cd packages/case-example-mock-types/dist/java

Then you can observe that the java package in packages/case-example-mock-types/dist/java does not contain the bundled dependency @contract-case/case-entities-internal.

(sorry I didn't have time to produce a minimal example - I don't think you'll actually need to reproduce it directly. Let me know if a minimal example would make a difference)

Interestingly, I think something resolves the file correctly - because if you do:

git clone git@github.com:case-contract-testing/contract-case.git
cd contract-case
npm install
cd packages/case-example-mock-types
npm run package

then you get:

[2023-06-05T22:12:14.201] [ERROR] jsii/package-info - Unable to find a JSII dependency named "@contract-case/test-equivalence-matchers" as "0.12.2". If you meant to include a non-JSII dependency, try adding it to bundledDependencies instead.
jsii compile [PROJECT_ROOT]

I believe this is because a clean checkout doesn't have @contract-case/test-equivalence-matchers built, so any attempt to resolve its entry point will fail.

My guess is that "do I have this module to bundle" is done differently to "ok now I will bundle it". Maybe?

SDK version used

jsii-pacmak@1.82.0 with jsii@5.0.9

Environment details (OS name and version, etc.)

node 18.13.0, npm 8.19.3, mac os 13.3.1 (a)

@TimothyJones TimothyJones added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 5, 2023
@TimothyJones
Copy link
Contributor Author

TimothyJones commented Jun 5, 2023

Per this discussion on slack, this is because jsii uses npm pack, which doesn't correctly respect workspaces. The linked npm bug above has been open since npm v7.

@RomainMuller
Copy link
Contributor

RomainMuller commented Jun 6, 2023

Yep. I suspect this is definitely because the bundled dependency is not present under crase-example-mock-types/node_modules and so npm pack silently omits it. I'm afraid this is a limitation of npm itself... and not quite a bug in jsii-pacmak, however I'm trying to see if we can work around the issue to get you out of that pit...

@RomainMuller
Copy link
Contributor

I believe I got the correct tarball created by npm pack by inserting the following pre- and post-pack scripts to the @contract-case/case-example-mock-types package... this isn't super pretty but it gets the job done (and you can substitute with something more elegant, in particular if building on Windows is a concern):

/// packages/case-example-mock-types/package.json
{
  // ...
  "scripts": {
    // ...
    "prepack": "mkdir -p node_modules/@contract-case && ln -sf ${PWD}/../case-entities node_modules/@contract-case/case-entities-internal",
    "postpack": "rm -rf node_modules",
    // ...
  },
  // ...
}

The prepack script is run just before npm pack creates the tarball, and creates a symbolic link to the bundled dependency in the right location, so that npm pack correctly discovers & bundles it up.

The postpack script runs just after npm pack created the tarball, and cleans the node_modules folder up (including the symlink inside it). This is optional but I don't like to leave cruft behind myself 😅

For convenience, here's the patch:

diff --git a/packages/case-example-mock-types/package.json b/packages/case-example-mock-types/package.json
index 1dd6379..ffbe498 100644
--- a/packages/case-example-mock-types/package.json
+++ b/packages/case-example-mock-types/package.json
@@ -23,6 +23,8 @@
     "build:docs": "jsii-docgen",
     "watch": "jsii -w",
     "prepackage": "rimraf dist",
+    "prepack": "mkdir -p node_modules/@contract-case && ln -sf ${PWD}/../case-entities node_modules/@contract-case/case-entities-internal",
+    "postpack": "rm -rf node_modules",
     "package": "jsii-pacmak",
     "publish:maven": "publib-maven",
     "test": "jest",

@RomainMuller
Copy link
Contributor

I'll close this for now, as this appears to be due to issues or missing features with rpm that we can't really address "in userland".

@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@icj217
Copy link
Contributor

icj217 commented Nov 15, 2023

In case someone is experiencing this issue but are not using workspaces/monorepos, double check that you don't accidentally have your bundled dependency registered as a dev dependency as well. Apparently npm pack will not bundle dependencies that exist in both places.

For example, yaml will not be bundled if your package.json looks like this:

{
      "devDependencies": {
          "yaml": "^2.1.3"
      },
      "dependencies": {
          "yaml": "^2.1.3"
      },
      "bundledDependencies": [
          "yaml"
      ]
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.
Projects
None yet
Development

No branches or pull requests

3 participants