web-ext: build with NODE_ENV=production

[wingdeans]

Description of changes

Fixes #256174

Setting NODE_ENV as an attribute results in this error:

Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'shelljs' imported from /build/source/scripts/bu>

Instead it is set in the preBuild hook, just before it is needed.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 23.11 Release Notes (or backporting 23.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

[dotlambda]

@wingdeans Do you want to add yourself as maintainer?

[wingdeans]

Setting NODE_ENV as an attribute results in this error:

Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'shelljs' imported from /build/source/scripts/bu>

Apparently this was because npm ci doesn't install devDependencies when NODE_ENV is set to production.

Using --include=dev in conjunction with NODE_ENV works and seems cleaner:

NODE_ENV = "production";
npmInstallFlags = "--include=dev";

@wingdeans Do you want to add yourself as maintainer?

No thanks

[lilyinstarlight]

I wonder if we should instead be using --omit=dev on the npmBuildHook so that way the build script is always run with NODE_ENV=production implicitly? (edit: perhaps that's too unintuitive... maybe just doing export NODE_ENV=production explicitly at the end of the configure hook is as better idea)

[lilyinstarlight]

I'm thinking of something like this:

diff --git a/pkgs/build-support/node/build-npm-package/hooks/npm-config-hook.sh b/pkgs/build-support/node/build-npm-package/hooks/npm-config-hook.sh
index 486b0c2f8372df..1936c32b6c07dd 100644
--- a/pkgs/build-support/node/build-npm-package/hooks/npm-config-hook.sh
+++ b/pkgs/build-support/node/build-npm-package/hooks/npm-config-hook.sh
@@ -109,6 +109,9 @@ npmConfigHook() {
     rm "$CACHE_MAP_PATH"
     unset CACHE_MAP_PATH
 
+    # Set build NODE_ENV to "production" by default *after* `npm ci` and `npm rebuild` to avoid an implicit `--omit=dev`
+    export NODE_ENV="${NODE_ENV-production}"
+
     if [ -n "${npmRoot-}" ]; then
       popd
     fi

I'll do some testing and possibly open that as a PR to fix the underlying issue. Feel free to take that diff for this PR, though, since you came up with the idea and I'm not attached to attribution

[jocelynthode]

Hey, any update on this PR? problem is unfortunately still there :(

[kirillrdy]

as per my comment in #256174 I think the issue has been resolved

[wingdeans]

Confirmed fixed