Fine-grained npm_install & yarn_install deps #302

gregmagolan · 2018-08-31T17:01:32Z

No description provided.

gregmagolan · 2018-09-05T03:35:30Z

internal/node/node.bzl

+         must now declare either an explicit node_modules attribute, or
+         list explicit deps[] or data[] dependencies on npm labels.
+
+         See http://FIXME""" % ctx.label)


@alexeagle Would be good not to introduce this breaking change. Could we detect if the node_modules was not set and there were no fine grained deps and use the default of @//:node_modules in that case?

Hmmm. I think I see what the reasoning is. We don't want to force users to define a //:node_modules filegroup if they move to fine grained deps. rollup_bundle will also need to be updated to removed the @//:node_modules default:

"node_modules": attr.label( doc = """Dependencies from npm that provide some modules that must be resolved by rollup.""", default = Label("@//:node_modules")),

Where were you thinking the http://FIXME would point?

the fixme should be a page on the wiki that explains the change and walk through how to do the upgrade

internal/jasmine_node_test/jasmine_runner.js

filipesilva · 2018-09-06T11:15:12Z

internal/npm_install/generate_build_file.js

+const fs = require('fs');
+const path = require('path');
+
+const DEBUG = console.error;


Shouldn't this be const DEBUG = false; like in node_loader.js? It doesn't seem to be used here though.

filipesilva · 2018-09-06T12:07:09Z

internal/npm_install/test/check.js

+function check(updateGolden = false) {
+  // We must change the directory to the BUILD file path
+  // so the generator is able to run
+  process.chdir('internal/npm_install/test')


If you change this to process.chdir(path.dirname(__filename)) you can remove the no_windows tag from //internal/npm_install/test:test

Brilliant. Thanks!

internal/npm_install/generate_build_file.js

gregmagolan · 2018-09-06T17:59:46Z

NOTE: This can't be released until we have a bazel release that supports @ in label names since scoped npm packages such as @angular/core and @types/node will have this character in their labels. Presently this feature is on bazel master but has not landed in a bazel release.

EDIT: This should be included in the 0.18.0 Bazel release scheduled for September ((bazelbuild/bazel#5963).

kitofans · 2018-09-06T19:03:36Z

internal/npm_install/generate_build_file.js

+        .reduce((a, b) => a.concat(b), [])
+  ];
+  let nestedPkgDirs = [];
+  pkgDirs.forEach(d => {


looking for nested packages is necessary but the way you've done it here isn't enough.

node modules can be arbitrarily nested, for example you could have:

node_modules/ bar/ baz/ foo/ node_modules/ bar/ node_modules/ baz/

It is important to traverse all the way down because the baz that resides three levels deep may have different dependencies than the baz that resides at the top-level node_modules (due to version differences). In this case, the transitive dependencies listed in the generated build file will not be accurate, since the baz found in pkgDirs will be the top-level one and not contain the correct dependencies.

I think you can just continue recursing down the found packages until none of them add any packages. Eventually the leaf packages will not have a nested node_modules directory.

@kitofans Thats a good point. I'll update the logic to handle arbitrary nested node_modules depth. Thanks!

an-kumar · 2018-09-06T20:13:18Z

w.r.t the label names including @, we could replace @ with at for the time being, would that work?

nimerritt · 2018-09-07T13:02:23Z

@gregmagolan @alexeagle This work is really interesting for me, since I just spent some time writing similar internal rules to track NPM dependencies at this fine-grained level as opposed to the whole node_modules. Originally I was generated a node_modules folder to pass into the rules, but realised that involved a lot of copying etc... since I couldn't figure out how to symlink directories easily. See https://groups.google.com/d/topic/bazel-discuss/JbA19d3vBis/discussion
In the end, I used the overloaded node module resolution via module_root and module_name. I had to use some macros to ensure those were set, since there isn't yet a common provider.

Our main motivation was to be able to generate NPM packages directly from ts_library-like targets. By tracking every NPM dependency, we can generate the dependencies stanza of the package.json file.

Rather than parsing the package.json file to figure out the transitive dependencies, I decided to just ignore hoisting with npm install some-package --global-style and use the entire directory as the file_group. I wrote a script to add workspace targets to pull in each package individually. Your approach (if I understood it correctly) pulls in all external NPM packages in one go and then figures out what is needed by each package. That is much more efficient than how I did it and also gives a nicer developer experiences 👍

I'm also curious about the default global @//:node_modules target, since that was used in the TypeScript rules which meant it kept rebuilding everything anytime those dependencies changed. To fix this, and another issue, I extracted a slimmed down version of your TypeScript rules in-order to avoid depending on the default @//:node_modules target. This is important since we use the node_modules folder at the root level of the project to support code-completion for the ts_server by constructing a symlink forest to the bazel-out directories. This was a fairly easy solution to https://github.com/bazelbuild/rules_typescript/issues/178

Any chance I can get access to the design document to understand if I can reuse more of this work? Interestingly enough, I also introduce a NodeModuleInfo provider with the exact same name :) Do you have some roadmap regarding these and the TypeScript rules?

gregmagolan · 2018-09-07T21:20:39Z

@an-kumar I'd rather not introduce the at- work-around since the 0.18.0 Bazel release that supports @ in label names should be out soon (bazelbuild/bazel#5963). We'd have to keep the at- as aliases going forward if it makes it in and you could have conflicts between a scoped folder such @foo and a package named at-foo.

gregmagolan · 2018-09-07T22:30:17Z

@nimerritt The design doc should be publicly accessible here, https://docs.google.com/document/d/1AfjHMLVyE_vYwlHSK7k7yW_IIGppSxsQtPm9PTr1xEo/edit. I'll be updating the typescript rules next to support idiomatic JS-only npm distributions for rules_typescript (also in this design doc).

pauldraper · 2018-09-10T06:29:29Z

Fixes #18

pauldraper · 2018-09-10T06:32:33Z

NOTE: This can't be released until we have a bazel release that supports @ in label names since scoped npm packages such as @angular/core and @types/node will have this character in their labels

Eh. Labels names can't have colons, but we've figured out how to do Maven dependencies.

I'd rather not introduce the at- work-around since the 0.18.0 Bazel release that supports @ in label names should be out soon

Bazel must be releasing on quicker schedule than in the past, since 0.17.0 isn't even out yet.

alexeagle

yum, really close!

alexeagle · 2018-09-11T21:45:36Z

.bazelci/presubmit.yml

@@ -24,13 +24,14 @@ platforms:
  windows:
    run_targets:
    - "@nodejs//:yarn"
+    build_flags:


looks like one of these is not used?
maybe just go back to the old content of this file, we should avoid a general mechanism for allowing things to be broken on windows

it was used but Filippe helped fix that windows test. will revert to old contents

README.md

WORKSPACE

alexeagle · 2018-09-11T21:52:00Z

docs/BUILD.bazel

@@ -30,4 +30,7 @@ skylark_doc(
    format = "html",
    site_root = "/rules_nodejs",
    strip_prefix = "internal/",
+    # Cannot build the docsite on Windows, see
+    # https://github.com/bazelbuild/skydoc/issues/58
+    tags = ["no_windows"],


examples/bazel_managed_deps/BUILD.bazel

alexeagle · 2018-09-11T22:09:03Z

internal/npm_install/test/check.js

+
+Update the golden file:
+
+      bazel run ${process.env['BAZEL_TARGET']}-accept`);


change to .accept

alexeagle · 2018-09-11T22:12:52Z

internal/npm_install/generate_build_file.js

+
+# Provide a lite version of the node_modules filegroup that includes
+# only js, d.ts and json files as well as the .bin folder. This can
+# be used in some cases to improve performance by reducing the number


might want to note bazelbuild/bazel#5769 here
since including files with no extension would fix the cases we know of where the "lite" group doesn't give all the files you need

internal/npm_install/generate_build_file.js

has been addressed

gregmagolan · 2018-09-13T23:00:55Z

Note to anyone who is going to try this out before it makes it into the next tagged release:

You'll need use the following rules_typescript commit: bazelbuild/rules_typescript#267 which should land soon but for now its on my fork of rules_typescript.

EDIT: bazelbuild/rules_typescript#267 landed so you can now use bazelbuild/rules_typescript HEAD

Bazel 0.18 warns about the tools/bazel.rc file Closes bazel-contrib#302 PiperOrigin-RevId: 217397650

gregmagolan force-pushed the fine_grained_deps branch 4 times, most recently from a8e8d99 to 2aff2b5 Compare September 5, 2018 03:32

gregmagolan changed the title ~~WIP: Fine-grained npm_install & yarn_install deps~~ Fine-grained npm_install & yarn_install deps Sep 5, 2018

gregmagolan requested review from alexeagle and filipesilva September 5, 2018 03:33

gregmagolan commented Sep 5, 2018

View reviewed changes

filipesilva previously requested changes Sep 6, 2018

View reviewed changes

kitofans reviewed Sep 6, 2018

View reviewed changes

internal/npm_install/generate_build_file.js Show resolved Hide resolved

kitofans reviewed Sep 6, 2018

View reviewed changes

gregmagolan force-pushed the fine_grained_deps branch from 2aff2b5 to 0d6f095 Compare September 6, 2018 22:31

This was referenced Sep 6, 2018

Read through package.json bin entry when resolving entry_point #205

Closed

internal/node/node_loader.js does not support NODE_PATH #304

Closed

gregmagolan force-pushed the fine_grained_deps branch 2 times, most recently from e4ff04c to 027603f Compare September 7, 2018 20:54

gregmagolan force-pushed the fine_grained_deps branch from 9e5de42 to aebfc38 Compare September 7, 2018 21:27

gregmagolan force-pushed the fine_grained_deps branch 2 times, most recently from 46d7daf to 6e180fd Compare September 11, 2018 21:20

alexeagle reviewed Sep 11, 2018

View reviewed changes

gregmagolan mentioned this pull request Sep 12, 2018

Update to rules_nodejs 0.12.4 bazelbuild/rules_typescript#267

Closed

gregmagolan force-pushed the fine_grained_deps branch 2 times, most recently from 628ba03 to 34a9674 Compare September 12, 2018 04:14

gregmagolan force-pushed the fine_grained_deps branch from 34a9674 to 9c4be2d Compare September 12, 2018 04:34

Fine-grained npm_install & yarn_install deps

b8955e8

gregmagolan force-pushed the fine_grained_deps branch from bcd45fc to b8955e8 Compare September 13, 2018 22:46

alexeagle approved these changes Sep 13, 2018

View reviewed changes

alexeagle merged commit 4554a9f into bazel-contrib:master Sep 13, 2018

alexeagle added a commit to alexeagle/rules_nodejs that referenced this pull request Oct 17, 2020

Move bazelrc to new location

509551f

Bazel 0.18 warns about the tools/bazel.rc file Closes bazel-contrib#302 PiperOrigin-RevId: 217397650

alexeagle added a commit to alexeagle/rules_nodejs that referenced this pull request Oct 18, 2020

Move bazelrc to new location

25e967b

Bazel 0.18 warns about the tools/bazel.rc file Closes bazel-contrib#302 PiperOrigin-RevId: 217397650

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fine-grained npm_install & yarn_install deps #302

Fine-grained npm_install & yarn_install deps #302

gregmagolan commented Aug 31, 2018

gregmagolan Sep 5, 2018 •

edited

Loading

gregmagolan Sep 5, 2018 •

edited

Loading

alexeagle Sep 5, 2018

filipesilva Sep 6, 2018

gregmagolan Sep 7, 2018

filipesilva Sep 6, 2018

gregmagolan Sep 7, 2018

gregmagolan commented Sep 6, 2018 •

edited

Loading

kitofans Sep 6, 2018 •

edited

Loading

gregmagolan Sep 7, 2018

an-kumar commented Sep 6, 2018

nimerritt commented Sep 7, 2018 •

edited

Loading

gregmagolan commented Sep 7, 2018

gregmagolan commented Sep 7, 2018

pauldraper commented Sep 10, 2018 •

edited

Loading

pauldraper commented Sep 10, 2018

alexeagle left a comment

alexeagle Sep 11, 2018

gregmagolan Sep 12, 2018

alexeagle Sep 11, 2018

gregmagolan Sep 12, 2018

alexeagle Sep 11, 2018

gregmagolan Sep 12, 2018

alexeagle Sep 11, 2018

gregmagolan Sep 13, 2018

gregmagolan commented Sep 13, 2018 •

edited

Loading


		Update the golden file:

		bazel run ${process.env['BAZEL_TARGET']}-accept`);

Fine-grained npm_install & yarn_install deps #302

Fine-grained npm_install & yarn_install deps #302

Conversation

gregmagolan commented Aug 31, 2018

gregmagolan Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

gregmagolan Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregmagolan commented Sep 6, 2018 • edited Loading

kitofans Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

an-kumar commented Sep 6, 2018

nimerritt commented Sep 7, 2018 • edited Loading

gregmagolan commented Sep 7, 2018

gregmagolan commented Sep 7, 2018

pauldraper commented Sep 10, 2018 • edited Loading

pauldraper commented Sep 10, 2018

alexeagle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregmagolan commented Sep 13, 2018 • edited Loading

gregmagolan Sep 5, 2018 •

edited

Loading

gregmagolan Sep 5, 2018 •

edited

Loading

gregmagolan commented Sep 6, 2018 •

edited

Loading

kitofans Sep 6, 2018 •

edited

Loading

nimerritt commented Sep 7, 2018 •

edited

Loading

pauldraper commented Sep 10, 2018 •

edited

Loading

gregmagolan commented Sep 13, 2018 •

edited

Loading