Add e2e tests/examples of transitive npm deps for ts_library - nodejs_binary interop #612

Globegitter · 2019-03-11T21:02:28Z

PR Type

What kind of change does this PR introduce?

Bugfix
Feature (please, look at the "Scope of the project" section in the README.md file)
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
Other... Please describe:

Tests for odd behaviour of ts_library <-> nodejs_binary (and rollup_bundle) interaction discussed and tested here: #283 (comment)

What is the current behavior?

It seems that under certain (or all?) circumstances nodejs_binary as well as rollup_bundle is not collecting transitive npm dependencies.

Issue Number: N/A

What is the new behavior?

For now this PR just adds tests but is not fixing the behaviour yet. I will also have a look at fixing the behaviour.

Does this PR introduce a breaking change?

Yes
No

Other information

cc @gregmagolan I was not sure where to best test this and if there is a better way of maybe using nodejs_test for this rather than nodejs_binary but let me know and happy to move this, clean this up etc.

Globegitter · 2019-03-14T07:52:21Z

Related #560

Globegitter · 2019-03-21T21:47:47Z

Should fix #560 as well as the issue discussed here: #283 (comment)

@gregmagolan would love some feedback on this.

alexeagle · 2019-03-26T22:11:16Z

packages/typescript/internal/build_defs.bzl

        # TODO(alexeagle): turn on strict deps checking when we have a real
        # provider for JS/DTS inputs to ts_library.
-        deps = [d for d in ctx.attr.deps if not NodeModuleInfo in d],
+        deps = [d for d in ctx.attr.deps if not NodeModuleInfo in d or (NodeModuleInfo in d and d[NodeModuleInfo].transitive == True)],


note, this line needs to go away anyway because it's causing bazelbuild/rules_typescript#381

I'll rebase around this though, glad you've added tests so I know I don't break it

alexeagle · 2019-03-26T22:12:20Z

internal/common/node_module_info.bzl

@@ -35,7 +36,13 @@ def _collect_node_modules_aspect_impl(target, ctx):

    if hasattr(ctx.rule.attr, "tags") and "NODE_MODULE_MARKER" in ctx.rule.attr.tags:
        nm_wksp = target.label.workspace_root.split("/")[1] if target.label.workspace_root else ctx.workspace_name
-        return [NodeModuleInfo(workspace = nm_wksp)]
+        return [NodeModuleInfo(workspace = nm_wksp, transitive = False)]


I would expect transitive to have a similar meaning to how a depset has a transitive argument (the depsets to hang off this one in the tree)
Can we find another name for this attribute? Really it's like a NodeModuleInfo for something that's not a node_module. Maybe it should just be a different provider instead of overloading this one?

Yeah maybe it does make sense to have a different provider as the attribute thing did feel a bit awkard. I'll have a bit of a think and another go at it.

alexeagle · 2019-03-26T22:12:48Z

Does this mean users no longer have to repeat deps on their rollup_bundle rule if a transitively included ts_library had the dep? That sounds great! We'll need to clean up stuff downstream.

Globegitter · 2019-03-27T21:14:59Z

Yep exactly this is what this is aimed at fixing, as it is also necessary for a correct implementation of #283

gregmagolan · 2019-03-28T15:17:39Z

internal/common/node_module_info.bzl

@@ -26,6 +26,7 @@
 NodeModuleInfo = provider(
    doc = "This provider contains information about npm dependencies installed with yarn_install and npm_install rules",
    fields = {
+        "transitive": "If true this dependency has transitive npm dependencies but is not and npm package itself",


Suggested change

"transitive": "If true this dependency has transitive npm dependencies but is not and npm package itself",

"transitive": "If true this dependency has transitive npm dependencies but is not an npm package itself",

gregmagolan · 2019-03-28T16:57:50Z

Nice. I did some initial debugging and this doesn't seem quite right and may not be hermetic. I'll do some more work on this later this evening.

Globegitter · 2019-03-29T08:32:39Z

Yes, I have not done very extensive testing and was looking for some input first, but will also start testing this with #283 as well as with our apps. Though which part are you referring to that might not be hermetic?

gregmagolan · 2019-03-29T15:13:13Z

Yes, I have not done very extensive testing and was looking for some input first, but will also start testing this with #283 as well as with our apps. Though which part are you referring to that might not be hermetic?

From an initial look it seemed like the transitive npm deps like date-fns are not added to the rollup action inputs but they are still resolved from their non-sandboxed location.

The code in rollup_bundle.bzl ends up adding the .d.ts files from ts_library deps that have NodeModuleInfo with transitive = true but not the transitive npm deps themselves..

    # Also include files from npm fine grained deps as inputs.
    # These deps are identified by the NodeModuleInfo provider.
    for d in ctx.attr.deps:
        if NodeModuleInfo in d:
            direct_inputs += _filter_js_inputs(d.files.to_list())

I'm going to play with it some more today as this would be a good one to get in and would unblock your #283 as well.

Globegitter · 2019-03-29T17:01:28Z

Aahh yes, to be honest I was actually surprised that it already worked by just adding the transitive NodeModuleInfo. So I suppose it somehow follows out a symlink somewhere? I have noticed that as well sometimes in our apps, that there can be different behaviour if node_modules exist in the developer's workspace or not. And so far I am assuming that some things somehow are not preserving symlinks.

Globegitter · 2019-04-03T07:37:24Z

Started debugging and it does indeed not seem to be hermetic. I do not find any trace of date-fns in the sandbox for the rollup action and the debug logs of rollup bundling show:

Rollup: resolving 'date-fns' from /home/markus/.cache/bazel/_bazel_markus/8464573ded3a95b161445c575d9af429/sandbox/linux-sandbox/1/execroot/e2e_typescript_3_1/bazel-out/k8-fastbuild/bin/bundle.es6/lib.js
Rollup: try to resolve 'date-fns' at '/home/markus/.cache/bazel/_bazel_markus/8464573ded3a95b161445c575d9af429/sandbox/linux-sandbox/1/execroot/e2e_typescript_3_1/bazel-out/k8-fastbuild/bin/bundle.es6/date-fns'
Rollup: allowing rollup to resolve 'date-fns' with node module resolution
Rollup: resolving './add_days/index.js' from /home/markus/.cache/bazel/_bazel_markus/8464573ded3a95b161445c575d9af429/external/npm/node_modules/date-fns/index.js

The last path is outside the sandbox. But need to look further into this to understand what exactly is happening.

Globegitter · 2019-04-03T15:10:41Z

Some further progress: I found rollup itself has a preserveSymlinks option which defaults to false so I set that and I also added the --preserve-symlinks-main just to make sure there is nothing related to symlinks. But that has not changed anything. I have found that the node-resolve plugin has a jail option, which is to limit module resolution just to a specific path. That seems like a sensible safeguard to me and setting this now the build is failing indeed. Still more work to be done.

gregmagolan · 2019-04-04T18:42:40Z

e2e/typescript_3.1/BUILD.bazel

+    ],
+)
+
+rollup_bundle(


Should change the test script in package.json to bazel build ... && bazel test ... so that these targets are built

I am pretty sure bazel test ... implicitly also builds all targets (unless --build_tests_only) is also specified. Let me verify this though.

Yep just verified. I had a failing rollup_bundle and bazel test ... failed on building it.

Ahhhh. Good to know. Thanks for verifying.

gregmagolan · 2019-04-04T18:46:17Z

internal/rollup/rollup.config.js

@@ -193,6 +194,7 @@ const config = {
    banner,
    format: 'TMPL_output_format',
  },
+  preserveSymlinks: true,


Nice. Do these preserve symlinks & jail changes work as intended to keep the resolve within the cwd()?

Yep, in this particular case it was really the jail attribute that brought the non-hermetic build to fail.

gregmagolan · 2019-04-04T19:02:02Z

internal/common/node_module_info.bzl

@@ -22,13 +22,36 @@ NodeModuleInfo = provider(
    },
 )

+NodeModuleSources = provider(


I suspect this is over complicating things here with two different node_modules providers and a NODE_MODULE_MARKER tag. This may be a good opportunity to remove the tag and create a single rule that is used instead of filegroup in generate_build_file.js so we can avoid the marker and have a single provider.

@kyliau recently added the ng_apf_library() rule which is used instead of filegroup when an angular package format is detected. We consolidate this work into a single rule and solve the transitive deps problem as well.

gregmagolan · 2019-04-06T18:03:38Z

Closing in favor of #675. Subset of changes from this PR will be ported over.

Format.

08d8125

Globegitter requested review from alexeagle, gregmagolan and kyliau as code owners March 11, 2019 21:02

Globegitter mentioned this pull request Mar 11, 2019

Add NodeJSSourcesInfo provider to allow for better layering in docker rules. #283

Closed

globegitter added 9 commits March 11, 2019 22:08

Bundle with dep is working.

38de67d

Fix package json.

4846436

Fix lint.

473f089

Fix lint 2.

20d0cab

Remove app library

a05534b

Add full support for transtivie npm dependency loading.

896df4b

Fix lint.

a228060

Add comment.

e3b8695

Remove print.

5cf4e8e

globegitter added 4 commits March 14, 2019 10:14

Expand comment.

3890729

Merge branch 'master' into mp-ts-nodejs-npm-example

ea41bce

Fix e2e test.

d1f48da

Merge branch 'master' into mp-ts-nodejs-npm-example

451232e

alexeagle reviewed Mar 26, 2019

View reviewed changes

googlebot added the cla: yes label Mar 27, 2019

gregmagolan reviewed Mar 28, 2019

View reviewed changes

Globegitter mentioned this pull request Apr 3, 2019

Reach outside of sandboxfs bazelbuild/sandboxfs#79

Closed

globegitter added 3 commits April 3, 2019 16:34

Merge branch 'master' into mp-ts-nodejs-npm-example

d6ef7f0

Add preserveSymlinks to rollup as well as jail.

6f970c2

Fix ordering.

b8de6f0

Globegitter mentioned this pull request Apr 3, 2019

Block access to files/paths outside of the sandbox bazelbuild/bazel#7936

Closed

globegitter added 4 commits April 3, 2019 23:39

Implement hacky way of providining transitive deps.

089cbce

Return info.

aac3532

Return info correctly.

54a8f6d

Implement NodeModuleSources provider.

29e668e

gregmagolan reviewed Apr 4, 2019

View reviewed changes

globegitter added 3 commits April 4, 2019 21:39

Fix missing workspace.

57fd800

Lint.

e483eee

Merge branch 'master' into mp-ts-nodejs-npm-example

417d9ab

gregmagolan mentioned this pull request Apr 6, 2019

Support for transitive npm dependencies #675

Merged

gregmagolan closed this Apr 6, 2019

Globegitter deleted the mp-ts-nodejs-npm-example branch April 16, 2019 09:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add e2e tests/examples of transitive npm deps for ts_library - nodejs_binary interop #612

Add e2e tests/examples of transitive npm deps for ts_library - nodejs_binary interop #612

Globegitter commented Mar 11, 2019 •

edited

Loading

Globegitter commented Mar 14, 2019

Globegitter commented Mar 21, 2019

alexeagle Mar 26, 2019

alexeagle Mar 26, 2019

Globegitter Mar 27, 2019

alexeagle commented Mar 26, 2019

Globegitter commented Mar 27, 2019

gregmagolan Mar 28, 2019

gregmagolan commented Mar 28, 2019

Globegitter commented Mar 29, 2019

gregmagolan commented Mar 29, 2019 •

edited

Loading

Globegitter commented Mar 29, 2019 •

edited

Loading

Globegitter commented Apr 3, 2019 •

edited

Loading

Globegitter commented Apr 3, 2019

gregmagolan Apr 4, 2019

Globegitter Apr 4, 2019

Globegitter Apr 4, 2019

gregmagolan Apr 4, 2019

gregmagolan Apr 4, 2019 •

edited

Loading

Globegitter Apr 4, 2019

gregmagolan Apr 4, 2019 •

edited

Loading

gregmagolan commented Apr 6, 2019

	"transitive": "If true this dependency has transitive npm dependencies but is not and npm package itself",
	"transitive": "If true this dependency has transitive npm dependencies but is not an npm package itself",

Add e2e tests/examples of transitive npm deps for ts_library - nodejs_binary interop #612

Add e2e tests/examples of transitive npm deps for ts_library - nodejs_binary interop #612

Conversation

Globegitter commented Mar 11, 2019 • edited Loading

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Globegitter commented Mar 14, 2019

Globegitter commented Mar 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeagle commented Mar 26, 2019

Globegitter commented Mar 27, 2019

Choose a reason for hiding this comment

gregmagolan commented Mar 28, 2019

Globegitter commented Mar 29, 2019

gregmagolan commented Mar 29, 2019 • edited Loading

Globegitter commented Mar 29, 2019 • edited Loading

Globegitter commented Apr 3, 2019 • edited Loading

Globegitter commented Apr 3, 2019

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 Apr 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregmagolan Apr 4, 2019 • edited Loading

Choose a reason for hiding this comment

gregmagolan commented Apr 6, 2019

Globegitter commented Mar 11, 2019 •

edited

Loading

gregmagolan commented Mar 29, 2019 •

edited

Loading

Globegitter commented Mar 29, 2019 •

edited

Loading

Globegitter commented Apr 3, 2019 •

edited

Loading

gregmagolan Apr 4, 2019 •

edited

Loading

gregmagolan Apr 4, 2019 •

edited

Loading