Rules_nodejs: @name//:node_modules only includes js / json / ts files

[UlysseM]

Hi,

As mentioned during the bazelcon, some files are not included within the node_modules (assets / css), causing build failure.

Also in the doc (https://bazelbuild.github.io/rules_nodejs/node/node.html#nodejs_binary) node_modules is marked as deprecated since 0.13, I think it should be 'discouraged'?

Thanks

[alexeagle]

Yeah it's here:
https://github.com/bazelbuild/rules_nodejs/blob/aafed6e6226dd28ab5fbd94a1056cd8014013872/internal/npm_install/generate_build_file.js#L93

the "catch-all" rule //:node_modules includes the "lite" version of the packages which only includes a few file extensions. we should probably make //:node_modules include everything, to limit breakage, and have //:node_modules_lite do the current behavior.

[gregmagolan]

Yes. We had decided to that as to limit the number of files as action in puts by default if users have a large node_modules. I agree that it may be better to have the default include all files with a //:node_modules_lite target providing the filtered node_modules.

With the change to multiple BUILD files, a user defined

yarn_install(
    name = "npm",
    package_json = "//:package.json",
    yarn_lock = "//:yarn.lock",
    manual_build_file_contents = """
filegroup(
  name = "node_modules_all",
  srcs = glob(["node_modules/**/*"]),
)
"""
)

will no longer work since each node_modules/pkg has a BUILD file in it now and the glob won't include nested packages.

[Globegitter]

Just saying I am running into the same thing - would have been nice for backwards compatibility if the node_module target kept all files and the node_modules_lite or whatever would have had this breaking behaviour - either way there is a straightforward fix I suppose.

Or another nice thing could have been a custom_file_endings attribute where the user can pass in which file endings should have been collected. But yeah not sure if it is worth to do anything for a deprecated feature.

[gfl-chris]

I've run into this a couple of times now and I've been working around it by explicitly adding the __pkg filegroup of the package that contains the asset, as in:

        "@name//:node_modules",
        "@name//node_modules/pkg1:pkg1__pkg",
        "@name//node_modules/pkg2:pkg2__pkg",
        "@name//node_modules/pkg3:pkg3__pkg",
        and so on...

It works just fine. (The __pkg target is really an alias for the __files target, so you could also use pkg1:pkg1__files etc.)

[gregmagolan]

All files are now included in the fine grained filegroups and coarse grained @name//:node_modules filegroup . For cases where you want to limit the number of file included, you can now the include_files attribute of yarn_install and npm_install. It defaults to an empty list [] which means to include all files.

Here's the full doc on include_files:

    "included_files": attr.string_list(
        doc = """List of file extensions to be included in the npm package targets.

        For example, [".js", ".d.ts", ".proto", ".json", ""].

        This option is useful to limit the number of files that are inputs
        to actions that depend on npm package targets. See
        https://github.com/bazelbuild/bazel/issues/5153.

        If set to an empty list then all files are included in the package targets.
        If set to a list of extensions, only files with matching extensions are
        included in the package targets. An empty string in the list is a special
        string that denotes that files with no extensions such as `README` should
        be included in the package targets.

        This attribute applies to both the coarse `@wksp//:node_modules` target
        as well as the fine grained targets such as `@wksp//foo`.""",
        default = [],
    ),