fix: tsc_wrapped depends on @npm//tsickle

[qzmfranklin]

PR Checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

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:

What is the current behavior?

With Bazel 0.22, the current tsc_wrapped target, when used to compile ts_library targets that need to yield es5/es6 files, causes runtime errors such as

ERROR: /home/zhongming/git/LogiOcean/examples/ts/googmodule/BUILD:5:1: Compiling TypeScript (devmode) //examples/ts/googmodule:googmodule failed (Exit 1): tsc_wrapped failed: error executing command
  (cd /home/zhongming/.cache/bazel/_bazel_zhongming/2aea414f4d422ad78870bb37da8563ec/execroot/logi && \
  exec env - \
    PATH=/bin:/usr/bin \
  bazel-out/host/bin/external/build_bazel_rules_typescript/@bazel/typescript/tsc_wrapped @@bazel-out/k8-fastbuild/bin/examples/ts/googmodule/googmodule_es5_tsconfig.json)
Execution platform: @bazel_tools//platforms:host_platform
Compilation failed Error: When setting bazelOpts { tsickle: true }, you must also add a devDependency on the tsickle npm package
    at emitWithTsickle (/home/zhongming/.cache/bazel/_bazel_zhongming/2aea414f4d422ad78870bb37da8563ec/execroot/logi/bazel-out/host/bin/external/build_bazel_rules_typescript/@bazel/typescript/tsc_wrapped.runfiles/npm/node_modules/@bazel/typescript/tsc_wrapped/tsc_wrapped.js:315:19)
    at runFromOptions (/home/zhongming/.cache/bazel/_bazel_zhongming/2aea414f4d422ad78870bb37da8563ec/execroot/logi/bazel-out/host/bin/external/build_bazel_rules_typescript/@bazel/typescript/tsc_wrapped.runfiles/npm/node_modules/@bazel/typescript/tsc_wrapped/tsc_wrapped.js:266:27)
    at runOneBuild (/home/zhongming/.cache/bazel/_bazel_zhongming/2aea414f4d422ad78870bb37da8563ec/execroot/logi/bazel-out/host/bin/external/build_bazel_rules_typescript/@bazel/typescript/tsc_wrapped.runfiles/npm/node_modules/@bazel/typescript/tsc_wrapped/tsc_wrapped.js:213:20)
    at Socket.<anonymous> (/home/zhongming/.cache/bazel/_bazel_zhongming/2aea414f4d422ad78870bb37da8563ec/execroot/logi/bazel-out/host/bin/external/build_bazel_rules_typescript/@bazel/typescript/tsc_wrapped.runfiles/npm/node_modules/@bazel/typescript/tsc_wrapped/worker.js:140:36)
    at Socket.emit (events.js:182:13)
    at emitReadable_ (_stream_readable.js:534:12)
    at process._tickCallback (internal/process/next_tick.js:63:19)

Issue Number: N/A

What is the new behavior?

The ts_library() rule works fine for all of its outputs.

Does this PR introduce a breaking change?

• Yes
• No

Other information

[qzmfranklin]

The error from circleci was:

ERROR: Analysis of target '//test_folder:hello' failed; build aborted: no such package '@npm//tsickle': BUILD file not found on package path

The only source of error I can think of for now is that tsickle is missing from package.json. But tsickle exists as part of the devDependencies in package.json.

When running on my own machines, it worked. Though, my setup was very different from that of the CI's/

Help needed. Thanks a lot in advance.

[alexeagle]

Hmm, this is tricky because the tsickle npm package is an optional dependency - you only need it when targeting closure compiler, otherwise we don't want to require it to be on disk.

Could you include a repro? I tried a couple things and don't get the same error.

[qzmfranklin]

Per 'want to avoid storing tsickle to disk': I would totally agree with both the conclusion and the reasoning so long as the resolution of this issue permits.

Alright, my setup. It might be daunting and I cannot easily share the code. I can but describe the procedure it took me to get the the state.

Basically, I copied the this repo. But the versions of (possibly relevant) dependencies are as follows:

rules_typescript 0.22.1
rules_nodejs 0.16.7
rules_go 0.17.0
rules_webtesting 0.3.0
angular 7.2.3
material2 7.3.0

Note that the angular and material2 versions are slightly different from what was used in that angular-bazel-example repo.

In my setup, the WORKSPACE file in all these Bazel repos are stripped to only containing the workspace(name = 'XXX') line. Everything in those WORKSPACE files are moved into the WORKSPACE file in my project, flattened. WORKSPACE files of transitive dependencies are also flattened into the WORKSPACE file in my project.

OK, this setup is already hard to reproduce (a lot of work). But if you succeed, you should be able to try out a (slightly modified version of) the examples/googomodule/... directory in this rules_typescript repo. And it shall give you error messages similar to what I reported in earlier comments.

I googled 'tsickle bazel tsc_wrapped' and found this issue. Indeed, this PR simply takes the demonstrated work from that issue.

I believe it has something to do with how the tsc_wrapped target was written. But I have not investigated it yet.

Last, I am happy with not being able to merge this PR with the hope that in the future someone else is going to provide a lightweight repro so that it becomes feasible for you to debug. If not, I am happy to just leave these information here to make it easy for others to search relevant info.

[qzmfranklin]

One more data point: I was able to fix this issue in my setup with this PR - tested with a 20k+ lines project.

[alexeagle]

Here's the repro:
https://github.com/alexeagle/repro_ts398/commits/master

[alexeagle]

okay this is blocked on a change in rules_nodejs to support a new bazelDynamicDependencies key, /cc @gregmagolan

[qzmfranklin]

Thanks @alexeagle for the deep dive and the repro!

I believe I learned a good amount of information to be happy with the situation.

Thanks for the patience and work.