Rules_nodejs: Yarn_install fails to reinstall when install input files change

[joeljeske]

I am using patch-package to patch a couple of npm packages. I have the issue where yarn_install does not appropriately run when I change or add new patches. I have attempted to declare the patches as dependencies in the data attr of the yarn_install but as the docs say:

If symlink_node_modules is True, this attribute is ignored since the dependency manager will run in the package.json location.

symlink_node_modules is true by default (and I think I want to keep it true). So what is the best practice way to declare files as dependency for this workspace rule?

[alexeagle]

@gregmagolan was just looking into something similar, I think there's a fix coming when https://github.com/bazelbuild/rules_nodejs/pull/1596 is released but I don't know if you need to take some action

[joeljeske]

I’ve read through that PR, and I’m still not quite sure how I would leverage that env var to ensure my scripts are tracked. Could you please explain how I might use it?

[joeljeske]

@alexeagle @gregmagolan
I don't think the change of using BAZEL_YARN_INSTALL is accomplishing what I need here (unless I don't understand).

My issue is that if I change some patches used by patch-packages, bazel will not re-run yarn install and apply those patches on other machines.

Is there a way that I can declare arbitrary labels as deps/data for my yarn install so it re-runs correctly?

[joeljeske]

@gregmagolan @alexeagle

It would seem that there are a few of open bug reports related to not invalidating the yarn/npm install

• https://github.com/bazelbuild/rules_nodejs/issues/1712
• https://github.com/bazelbuild/rules_nodejs/issues/1714
• https://github.com/bazelbuild/rules_nodejs/issues/1311
• https://github.com/bazelbuild/rules_nodejs/issues/1601

I think the root cause of these issues is that the data attr for yarn_install and npm_install is not respected if symlink_node_modules which is the preferred installation method.

I think we need a method to add arbitrary files that when changed, invalidates the install and forces a new one to occur. Would you guys agree/disagree?

[joeljeske]

Any thoughts on this @gregmagolan or @alexeagle?

Currently I have to make sure change an arbitrary dependency whenever I update a patch or else the patch may not be applied for everyone. This leads to a confusing process.

[gregmagolan]

@joeljeske Thanks for gather all the issue numbers. I'll put this on my list for as it looks like its causing a lot of pain. Non breaking so I'll cut a 1.6.2 once fixed.

[gregmagolan]

@joeljeske I was unable to reproduce the issue with data not causing yarn_install to re-run even with symlink_node_modules = True. Changing a file in data always caused the repo run to rerun all the way back to Bazel 1.0.0. I tested on Linux, OSX & Windows to make sure the behavior was consistent.

I did, however, fix up the doc string of data to be more clear and I fixed #1714 in #1866. I also changed the logic so that data is not ignored but that the symlinks to package.json & the lock file and the copying of data files is always done. This is more principled even if the files are unused in the external repo when symlink_node_modules = True as the external repository does depend on them. It also has the side-effect of Bazel checking that the data labels are valid files as it is easy to get a label wrong and that is currently silently ignored.

[gregmagolan]

On further thought, I think what you're observing is that the Label passed to data is not correct and because it is not used and not evaluated then Bazel does not error with with //your/label is not a regular file. Its a very easy error to make as the label must be //:patches/jest-haste-map+25.3.0.patch and not //patches:jest-haste-map+25.3.0.patch for example if //patches is not a package.

With the fix in #1866, the labels in data will always be evaluated so if they are not correct Bazel will error out.

[joeljeske]

Ok, thank you for the detailed explanations. There was an issue with the label to data that I corrected. That, combined with the docs mentioning that it is not used, led me to not look further at it. (I have gotten spoiled by Bazel typically erroring fast on typos and didn't even consider it)

[joeljeske]

Well, I spoke a little too soon @gregmagolan . I did have the issue you mentioned with an incorrect label. After fixing it, it still did not fix the issue. I had used a filegroup of the patches with a glob(["patches/*.patch"]) It seems that using this approach does not have trigger the reinstall (although it probably will after the fix #1866). I have since converted to explicilty listing out each file in the data, but I would like to see the support for a filegroup as it is a better long term strategy.

[gregmagolan]

@joeljeske AFAIK, globs don't work in the WORKSPACE file. I think this is a Bazel limitation although at the moment I can't remember the reason.