fix(builtin): fix regression in 1.6.0 in linker linking root package when under runfiles #1854

gregmagolan · 2020-04-29T14:39:51Z

Resolves #1823 which had a repro https://github.com/purkhusid/ts-jest-repro.

NB: The repo uses ts_library devmode UMD output so the relative require in the source code will be transformed to an absolute require which will include the workspace name.

In 1.5.0:

.../sandbox/darwin-sandbox/23/execroot/jest_repro/bazel-out/darwin-fastbuild/bin/test.sh.runfiles/jest_repro/lib.test.js

const lib_1 = require("jest_repro/lib");

works as it finds

.../sandbox/darwin-sandbox/23/execroot/jest_repro/bazel-out/darwin-fastbuild/bin/test.sh.runfiles/jest_repro/node_modules/jest_repro/lib.js

where test.sh.runfiles/jest_repro/node_modules/jest_repro is a symlink to the root of the output tree setup by the linker:

lrwxr-xr-x  1 greg  wheel  165 29 Apr 05:56 .../sandbox/darwin-sandbox/6/execroot/jest_repro/bazel-out/darwin-fastbuild/bin/test.sh.runfiles/jest_repro/node_modules/jest_repro -> .../sandbox/darwin-sandbox/6/execroot/jest_repro/bazel-out/darwin-fastbuild/bin/test.sh.runfiles/jest_repro

In 1.6.0:

Bug introduced in #1850 (thanks for tracking down the culprit @alexeagle) where the root symlink is not created properly when under runfiles (such as a jest_test context).

Fix is:

          let runfilesPath = modulePath;
          if (runfilesPath.startsWith(`${bin}/`)) {
            runfilesPath = runfilesPath.slice(bin.length + 1);
+          } else if (runfilesPath === bin) {
+            runfilesPath = '';
          }

in link_node_modules.ts.

NB: This root workspace symlink will be removed in 2.0 as it causes other unrelated issues. PR for that change is in progress #1797. In 2.0, we recommend either using the prodmode esm output of ts_library for jest_test or switching to ts_project where the output flavor is controlled by your tsconfig. An example of using ts_library esm output of ts_library (output_group 'es6_sources') is under /internal/linker/test/issue_1823_use_ts_library_esm. It require babel-jest to transform the esm to commonjs. Alternately, you can control the 'es5_sources' module format of ts_library with the devmode_module but this can lead to other issues such as the output no longer being compatible with ts_devserver and karma_web_test which require named-UMD or named-AMD.

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?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

@alexeagle

…when under runfiles Resolves bazel-contrib#1823 which had a repro https://github.com/purkhusid/ts-jest-repro. NB: The repo uses ts_library devmode UMD output so the relative require in the source code will be transformed to an absolute require which will include the workspace name. In 1.5.0: ``` .../sandbox/darwin-sandbox/23/execroot/jest_repro/bazel-out/darwin-fastbuild/bin/test.sh.runfiles/jest_repro/lib.test.js const lib_1 = require("jest_repro/lib"); ``` works as it finds ``` .../sandbox/darwin-sandbox/23/execroot/jest_repro/bazel-out/darwin-fastbuild/bin/test.sh.runfiles/jest_repro/node_modules/jest_repro/lib.js ``` where `test.sh.runfiles/jest_repro/node_modules/jest_repro` is a symlink to the root of the output tree setup by the linker: ``` lrwxr-xr-x 1 greg wheel 165 29 Apr 05:56 .../sandbox/darwin-sandbox/6/execroot/jest_repro/bazel-out/darwin-fastbuild/bin/test.sh.runfiles/jest_repro/node_modules/jest_repro -> .../sandbox/darwin-sandbox/6/execroot/jest_repro/bazel-out/darwin-fastbuild/bin/test.sh.runfiles/jest_repro ``` In 1.6.0: Bug introduced in bazel-contrib#1850 (thanks for tracking down the culprit @alexeagle) where the root symlink is not created properly when under runfiles (such as a jest_test context). Fix is: ``` let runfilesPath = modulePath; if (runfilesPath.startsWith(`${bin}/`)) { runfilesPath = runfilesPath.slice(bin.length + 1); + } else if (runfilesPath === bin) { + runfilesPath = ''; } ``` in `link_node_modules.ts`. NB: This root workspace symlink will be removed in 2.0 as it causes other unrelated issues. PR for that change is in progress bazel-contrib#1797. In 2.0, we recommend either using the prodmode esm output of ts_library for jest_test or switching to ts_project where the output flavor is controlled by your tsconfig. An example of using ts_library esm output of ts_library (output_group 'es6_sources') is `under /internal/linker/test/issue_1823_use_ts_library_esm`. It require babel-jest to transform the esm to commonjs. Alternately, you can control the 'es5_sources' module format of ts_library with the `devmode_module` but this can lead to other issues such as the output no longer being compatible with ts_devserver and karma_web_test which require named-UMD or named-AMD.

alexeagle

thanks for tracking it down!

jbedard · 2020-04-29T15:24:56Z

packages/rollup/test/integration/far/a/b/c/tsconfig.json

@@ -4,6 +4,7 @@
    "strict": true,
    "target": "es2015",
    "module": "esnext",
-    "moduleResolution": "node"
+    "moduleResolution": "node",
+    "types": []


Why add this everywhere?

To keep Windows CI from breaking. Explained in commit message:
This prevents @types packages added to the root package.json from inadvertently breaking tests on buildkite Windows CI where there is no sandboxing.

https://buildkite.com/bazel/rules-nodejs-nodejs/builds/6691#41586486-08b9-447a-aed1-6f9fff24121b

./../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jasmine/ts3.1/index.d.ts(16,1): error TS6200: Definitions of the following identifiers conflict with those in another file: beforeAll, beforeEach, afterAll, afterEach, describe, fdescribe, xdescribe, it, fit, xit, expect, DEFAULT_TIMEOUT_INTERVAL, CustomMatcherFactory, CustomEqualityTester -- | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jasmine/ts3.1/index.d.ts(253,15): error TS2428: All declarations of 'ArrayContaining' must have identical type parameters. | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jasmine/ts3.1/index.d.ts(257,15): error TS2428: All declarations of 'ObjectContaining' must have identical type parameters. | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jasmine/ts3.1/index.d.ts(304,9): error TS2687: All declarations of 'message' must have identical modifiers. | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jasmine/ts3.1/index.d.ts(730,15): error TS2428: All declarations of 'SpyAnd' must have identical type parameters. | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jasmine/ts3.1/index.d.ts(747,15): error TS2428: All declarations of 'Calls' must have identical type parameters. | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jasmine/ts3.1/index.d.ts(766,15): error TS2428: All declarations of 'CallInfo' must have identical type parameters. | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jest/index.d.ts(34,1): error TS6200: Definitions of the following identifiers conflict with those in another file: beforeAll, beforeEach, afterAll, afterEach, describe, fdescribe, xdescribe, it, fit, xit, expect, DEFAULT_TIMEOUT_INTERVAL, CustomMatcherFactory, CustomEqualityTester | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jest/index.d.ts(1354,46): error TS2314: Generic type 'ArrayContaining<T>' requires 1 type argument(s). | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jest/index.d.ts(1355,45): error TS2314: Generic type 'ObjectContaining<T>' requires 1 type argument(s). | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jest/index.d.ts(1381,15): error TS2428: All declarations of 'ArrayContaining' must have identical type parameters. | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jest/index.d.ts(1387,15): error TS2428: All declarations of 'ObjectContaining' must have identical type parameters. | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jest/index.d.ts(1390,9): error TS2386: Overload signatures must all be optional or required. | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jest/index.d.ts(1396,14): error TS2314: Generic type 'SpyAnd<Fun>' requires 1 type argument(s). | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jest/index.d.ts(1397,16): error TS2314: Generic type 'Calls<Fun>' requires 1 type argument(s). | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jest/index.d.ts(1403,15): error TS2428: All declarations of 'SpyAnd' must have identical type parameters. | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jest/index.d.ts(1436,15): error TS2428: All declarations of 'Calls' must have identical type parameters. | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jest/index.d.ts(1462,16): error TS2314: Generic type 'CallInfo<Fun>' requires 1 type argument(s). | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jest/index.d.ts(1467,23): error TS2314: Generic type 'CallInfo<Fun>' requires 1 type argument(s). | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jest/index.d.ts(1472,18): error TS2314: Generic type 'CallInfo<Fun>' requires 1 type argument(s). | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jest/index.d.ts(1479,15): error TS2428: All declarations of 'CallInfo' must have identical type parameters. | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jest/index.d.ts(1487,9): error TS2717: Subsequent property declarations must have the same type. Property 'args' must be of type 'Parameters<Fun>', but here has type 'any[]'. | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jest/index.d.ts(1491,9): error TS2717: Subsequent property declarations must have the same type. Property 'returnValue' must be of type 'ReturnType<Fun>', but here has type 'any'. | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jest/index.d.ts(1495,9): error TS2374: Duplicate string index signature. | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jest/index.d.ts(1515,9): error TS2687: All declarations of 'message' must have identical modifiers. | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jest/index.d.ts(1515,9): error TS2717: Subsequent property declarations must have the same type. Property 'message' must be of type 'string \| undefined', but here has type 'string \| (() => string)'. | ../../../bk-windows-c4d7/bazel/rules-nodejs-nodejs/node_modules/@types/jest/index.d.ts(1520,9): error TS2375: Duplicate number index signature.

Adding @types/jest blew things up and it is generally good practice to be explicit about what types are needed. The other option was skipLibCheck: true which is not ideal as it skips checks for all .d.ts files.

This prevents @types packages added to the root package.json from inadvertently breaking tests on buildkite Windows CI where there is no sandboxing.

gregmagolan requested review from alexeagle and soldair as code owners April 29, 2020 14:39

googlebot added the cla: yes label Apr 29, 2020

alexeagle approved these changes Apr 29, 2020

View reviewed changes

gregmagolan requested a review from jbedard as a code owner April 29, 2020 15:15

jbedard reviewed Apr 29, 2020

View reviewed changes

jbedard approved these changes Apr 29, 2020

View reviewed changes

gregmagolan force-pushed the fix-1823 branch 4 times, most recently from 29295c1 to c74ebaf Compare April 29, 2020 18:59

test: be explicit about which types are used in ts_project tests

199acf4

This prevents @types packages added to the root package.json from inadvertently breaking tests on buildkite Windows CI where there is no sandboxing.

gregmagolan force-pushed the fix-1823 branch 2 times, most recently from 3e13dbe to ce39743 Compare April 29, 2020 22:39

test: copy_to_bin shenanigans for Windows CI

0407d25

gregmagolan force-pushed the fix-1823 branch from ce39743 to 0407d25 Compare April 29, 2020 23:25

gregmagolan merged commit 9f541ce into bazel-contrib:master Apr 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(builtin): fix regression in 1.6.0 in linker linking root package when under runfiles #1854

fix(builtin): fix regression in 1.6.0 in linker linking root package when under runfiles #1854

gregmagolan commented Apr 29, 2020

alexeagle left a comment

jbedard Apr 29, 2020

gregmagolan Apr 29, 2020

gregmagolan Apr 29, 2020

gregmagolan Apr 29, 2020

gregmagolan Apr 29, 2020

fix(builtin): fix regression in 1.6.0 in linker linking root package when under runfiles #1854

fix(builtin): fix regression in 1.6.0 in linker linking root package when under runfiles #1854

Conversation

gregmagolan commented Apr 29, 2020

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

alexeagle left a comment

Choose a reason for hiding this comment

jbedard Apr 29, 2020

Choose a reason for hiding this comment

gregmagolan Apr 29, 2020

Choose a reason for hiding this comment

gregmagolan Apr 29, 2020

Choose a reason for hiding this comment

gregmagolan Apr 29, 2020

Choose a reason for hiding this comment

gregmagolan Apr 29, 2020

Choose a reason for hiding this comment