simondotm / nx-firebase

Firebase plugin for Nx Monorepos
https://www.npmjs.com/package/@simondotm/nx-firebase
MIT License
180 stars 31 forks source link

`@nx/esbuild` Does not output pruned `package-lock.json` #161

Closed simondotm closed 10 months ago

simondotm commented 1 year ago

When building a firebase function, @nx/esbuild will output a package.json in dist containing the functions dependencies due to generatePackageJson: true. However, the @nx/esbuild plugin does not output a pruned package-lock.json file which other Nx plugins like @nx/webpack and @nx/node do.

Without a package-lock.json file in dist, when firebase deploys this function, it may run npm ci and generate dependencies that are different from the ones in the workspace root package-lock.json, and ultimately fail to deploy if there are incompatible peer deps.

This issue lies with Nx and requires a fix on their side.

ciriousjoker commented 1 year ago

We're trying to upgrade to v2 atm and we ran into an issue where the generated package-lock.json in dist/ contains no dependencies or devDependencies even though the function code imports from "firebase-admin".

This fails during deployment with this error:

original": { "name": "FirebaseError", "children": [], "exit": 1, "message": "Function failed on loading user code. This is likely due to a bug in the user code. Error message: Provided module can't be loaded.\nIs there a syntax error in your code?\nDetailed stack trace: Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'firebase-admin' imported from /workspace/main.js\n at new NodeError (node:internal/errors:387:5)\n at packageResolve (node:internal/modules/esm/resolve:852:9)\n at moduleResolve (node:internal/modules/esm/resolve:901:20)\n at defaultResolve (node:internal/modules/esm/resolve:1115:11)\n at nextResolve (node:internal/modules/esm/loader:163:28)\n at ESMLoader.resolve (node:internal/modules/esm/loader:841:30)\n at ESMLoader.getModuleJob (node:internal/modules/esm/loader:424:18)\n at ModuleWrap. (node:internal/modules/esm/module_job:76:40)\n at link (node:internal/modules/esm/module_job:75:36)\nCould not load the function, shutting down.. Please visit https://cloud.google.com/functions/docs/troubleshooting for in-depth troubleshooting documentation.", "status": 3, "code": 3 }

Is this related to this or is this a separate issue?

We're using nx 17 if that matters:

"@nx/angular": "17.0.0",
"@nx/cypress": "17.0.0",
"@nx/devkit": "17.0.0",
"@nx/esbuild": "17.0.0",
"@nx/eslint": "17.0.0",
"@nx/eslint-plugin": "17.0.0",
"@nx/jest": "17.0.0",
"@nx/js": "17.0.0",
"@nx/linter": "17.0.0",
"@nx/node": "17.0.0",
"@nx/workspace": "17.0.0",
ciriousjoker commented 1 year ago

I just realized that there's a package.json in the apps/my-function directory without any dependencies. Are we supposed to add those there? This seems really tedious given that all the dependencies in the correct versions are already listed in the top level package.json and the import statements are also in the code as well.

simondotm commented 1 year ago

@ciriousjoker

I just realized that there's a package.json in the apps/my-function directory without any dependencies. Are we supposed to add those there? This seems really tedious given that all the dependencies in the correct versions are already listed in the top level package.json and the import statements are also in the code as well.

No, there is no requirement to manage dependencies manually - that would indeed be tedious!

The @nx/esbuild project config for function has generatePackageJson: true which should automatically emit a package.json in dist containing dependencies used by the function app. The empty package.json in the functions project is a template.

It is curious that you are seeing a package-lock.json file in your dist folder, since this issue was raised because we weren't seeing this file being emitted by Nx. Did you mean package.json ? If not, then I wonder if Nx have updated the behaviour of @nx/esbuild in V17 to now emit package-lock.json files (albeit apparently broken for you)...

ciriousjoker commented 1 year ago

Yes, it's generating both files, although there are no dependencies in either.

If I add dependencies to the template, they're also present in the dist folder.

simondotm commented 1 year ago

Ok thanks for updating. Clearly there's something up with Nx dependency generation, so this is something that needs to be looked into with Nx v17.

I've not got a lot of spare time at the moment, so dont know how quickly I can look at this.

My approach would be to put some console.log in the node_modules/@nx/esbuild/... source files to see what dependencies it is seeing (and not outputting for whatever reason)

Relevant Nx source code is here if you need this fixing quickly and maybe have some time to investigate: https://github.com/nrwl/nx/blob/ac038e353c32b96655aa8c6145d6d0666361fe76/packages/esbuild/src/executors/esbuild/esbuild.impl.ts#L75

simondotm commented 1 year ago

At least from a quick glance at that code, it does seem that their esbuild plugin now seems to be coded to emit package-lock.json files anyway, which is the original topic for this issue, so thats some sort of progress. Not much use if it doesnt work though 😞

simondotm commented 1 year ago

@ciriousjoker bit of a long shot but could you try adding updateBuildableProjectDepsInPackageJson: true to the options section of your build target for your functions project.json ?

I'm suspicious of this code here: https://github.com/nrwl/nx/blob/ac038e353c32b96655aa8c6145d6d0666361fe76/packages/js/src/utils/package-json/update-package-json.ts#L63C15-L63C54

ciriousjoker commented 1 year ago

@simondotm This worked! CleanShot 2023-10-21 at 22 12 19@2x It wasn't there before if you are wondering.

Now there's a package.json and a (seemingly matching) package-lock.json in build/. The dependency list is a bit longer than necessary. I think with perfect tree-shaking it could have figured out that even though I imported a shared library, the only type I've imported doesn't need any external code and is just one line.

But hey, it works! Thank's a lot for the help. I'm sorry if I unintentionally hijacked this issue with an unrelated question.

simondotm commented 1 year ago

Thats great news. I've seen this option lurking around in various discussions, but it never seemed to be documented anywhere. At the time of writing its not mentioned on the @nx/esbuild docs: https://nx.dev/nx-api/esbuild/executors/esbuild

It's not an option listed in the esbuild plugin schema either, so I dont really know why or how this works, but yeah, let's take the win 😅

The dependency list is a bit longer than necessary. I think with perfect tree-shaking it could have figured out that even though I imported a shared library, the only type I've imported doesn't need any external code and is just one line.

It's worth noting that because it's Nx that is populating this list, it will likely be every dependency that Nx believes your code directly or indirectly accesses - so any imports from libraries you have will be likely included and causing this bloat. At least it is correct though, even if overkill.

Ideally it would be esbuild that writes this package.json because the dependency list after tree shaking as you say should be typically much smaller, but that would need looking into. I remember looking into this a while ago and used an esbuild plugin to write it, but then Nx just overwrites it after compilation is complete. 😕

I doubt this has much impact in practice, probably just means that functions will be using up a bit more firebase storage budget since the container for your function will have a larger node_modules than necessary.

I'm sorry if I unintentionally hijacked this issue with an unrelated question.

Not at all, this is totally on topic with the OP

simondotm commented 1 year ago

It gets wierder - the migration tool for Nx 17.0.0 has an explicit migration to remove this build option from all targets! 🔥

https://github.com/nrwl/nx/blob/ac038e353c32b96655aa8c6145d6d0666361fe76/packages/js/src/migrations/update-17-0-0/remove-deprecated-build-options.ts#L27C37-L27C37

brendanowens commented 1 year ago

I am running into this same thing. If my functions package.json is the basic boilerplate, and I run nx build [functions-app], the package.json and the package-lock.json in the dist folder both contain no dependencies.

If I add a few dependencies to my functions package.json such as this:

{
  "name": "layer-functions",
  "description": "Firebase Function, auto generated by @simondotm/nx-firebase",
  "scripts": {
    "test": "nx test layer-functions",
    "lint": "nx lint layer-functions",
    "build": "nx build layer-functions",
    "deploy": "nx deploy layer-functions"
  },
  "type": "module",
  "engines": {
    "node": "16"
  },
  "dependencies": {
    "firebase-admin": "11.11.0",
    "firebase-functions": "4.5.0",
    "firebase-tools": "12.8.0",
    "xhr2": "^0.2.1"
  },
  "devDependencies": {},
  "private": true
}

Then the package-lock.json generated in dist includes all of the dependencies needed BUT I get the error (with the package not found always being the first one listed in package.json):

Original error: Pruned lock file creation failed. The following package was not found in the root lock file: firebase-admin@11.11.0

When deploying, the mis-match between the package-lock and package.json is preventing the deploy from succeeding.

I have tried upgrading to NX 17, but that did not resolve the issue

brendanowens commented 1 year ago

This looks to be a bug in @nx/esbuild versions > 16.7.4. I locked in on 16.7.4 and my package.json generates as expected. Here is the issue in their repo.

simondotm commented 10 months ago

I can confirm that package-lock.json files seem to be generated correctly in dist from Nx 17+

simondotm commented 10 months ago

Closing this one

danielmahon commented 8 months ago

Seeing this issue again in Nx v18+

robert14k commented 3 months ago

I am also still seeing this with trying to upgrade from @nx/esbuild 16.x.x to 19.x.x