Skip to content

Commit

Permalink
fix(builtin): fix regression in 1.6.0 in linker linking root package …
Browse files Browse the repository at this point in the history
…when under runfiles

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.
  • Loading branch information
gregmagolan committed Apr 29, 2020
1 parent a167311 commit b4149d8
Show file tree
Hide file tree
Showing 15 changed files with 346 additions and 5 deletions.
3 changes: 3 additions & 0 deletions internal/linker/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,9 @@ function main(args, runfiles) {
if (runfilesPath.startsWith(`${bin}/`)) {
runfilesPath = runfilesPath.slice(bin.length + 1);
}
else if (runfilesPath === bin) {
runfilesPath = '';
}
const externalPrefix = 'external/';
if (runfilesPath.startsWith(externalPrefix)) {
runfilesPath = runfilesPath.slice(externalPrefix.length);
Expand Down
2 changes: 2 additions & 0 deletions internal/linker/link_node_modules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -601,6 +601,8 @@ export async function main(args: string[], runfiles: Runfiles) {
let runfilesPath = modulePath;
if (runfilesPath.startsWith(`${bin}/`)) {
runfilesPath = runfilesPath.slice(bin.length + 1);
} else if (runfilesPath === bin) {
runfilesPath = '';
}
const externalPrefix = 'external/';
if (runfilesPath.startsWith(externalPrefix)) {
Expand Down
22 changes: 22 additions & 0 deletions internal/linker/test/issue_1823/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
load("@npm_bazel_typescript//:index.bzl", "ts_library")
load(":ts_jest_test.bzl", "ts_jest_test")

ts_library(
name = "lib",
srcs = [
"lib.ts",
],
deps = [
],
)

ts_jest_test(
name = "test",
srcs = [
"lib.test.ts",
],
jest_config = "jest.config.js",
deps = [
":lib",
],
)
3 changes: 3 additions & 0 deletions internal/linker/test/issue_1823/jest.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
module.exports = {
testEnvironment: 'node',
};
7 changes: 7 additions & 0 deletions internal/linker/test/issue_1823/lib.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import {doStuff} from './lib';

describe('doStuff', () => {
it('should do some stuff', () => {
expect(doStuff('boom')).toContain('boom');
});
});
3 changes: 3 additions & 0 deletions internal/linker/test/issue_1823/lib.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export function doStuff(a: string): string {
return a
}
35 changes: 35 additions & 0 deletions internal/linker/test/issue_1823/ts_jest_test.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
"""Simple macro around jest_test"""

load("@npm//jest-cli:index.bzl", _jest_test = "jest_test")
load("@npm_bazel_typescript//:index.bzl", "ts_library")

def ts_jest_test(name, srcs, jest_config, deps = [], data = [], **kwargs):
"""A macro around the autogenerated jest_test rule that takes typescript sources
Uses ts_library devmode UMD output"""

ts_library(
name = "%s_ts" % name,
srcs = srcs,
data = data,
deps = deps + ["@npm//@types/jest"],
)
native.filegroup(
name = "%s_umd" % name,
srcs = [":%s_ts" % name],
output_group = "es5_sources",
)

args = [
"--no-cache",
"--no-watchman",
"--ci",
]
args.extend(["--config", "$(rootpath %s)" % jest_config])

_jest_test(
name = name,
data = [jest_config, ":%s_umd" % name] + deps + data,
args = args,
**kwargs
)
12 changes: 12 additions & 0 deletions internal/linker/test/issue_1823_use_ts_library_esm/.babelrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"presets": [
[
"@babel/preset-env",
{
"targets": {
"node": "current",
},
},
],
]
}
31 changes: 31 additions & 0 deletions internal/linker/test/issue_1823_use_ts_library_esm/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
load("@npm_bazel_typescript//:index.bzl", "ts_library")
load(":ts_jest_test.bzl", "ts_jest_test")

ts_library(
name = "lib",
srcs = [
"lib.ts",
],
# NB: hacky hidden configuration setting so that es6_sources does not include tsickle
# .externs.js outputs
runtime = "nodejs",
deps = [
"@npm//@types/node",
],
)

ts_jest_test(
name = "test",
srcs = [
"lib.test.ts",
],
data = [
".babelrc",
],
jest_config = "jest.config.js",
deps = [
":lib",
"@npm//@babel/preset-env",
"@npm//babel-jest",
],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
module.exports = {
testEnvironment: 'node',
transform: {'^.+\\.mjs?$': 'babel-jest'},
testMatch: ['**/?(*.)(spec|test).?(m)js?(x)'],
moduleFileExtensions: ['js', 'mjs'],
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import {doStuff} from './lib';

describe('doStuff', () => {
it('should do some stuff', () => {
expect(doStuff('boom')).toContain('boom');
});
});
3 changes: 3 additions & 0 deletions internal/linker/test/issue_1823_use_ts_library_esm/lib.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export function doStuff(a: string): string {
return a
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
"""Simple macro around jest_test"""

load("@npm//jest-cli:index.bzl", _jest_test = "jest_test")
load("@npm_bazel_typescript//:index.bzl", "ts_library")

def ts_jest_test(name, srcs, jest_config, deps = [], data = [], **kwargs):
"""A macro around the autogenerated jest_test rule that takes typescript sources
Uses ts_library prodmode ems output"""

ts_library(
name = "%s_ts" % name,
srcs = srcs,
data = data,
deps = deps + ["@npm//@types/jest"],
# NB: hacky hidden configuration setting so that es6_sources does not include tsickle
# .externs.js outputs
runtime = "nodejs",
)
native.filegroup(
name = "%s_esm" % name,
srcs = [":%s_ts" % name],
output_group = "es6_sources",
)

args = [
"--no-cache",
"--no-watchman",
"--ci",
]
args.extend(["--config", "$(rootpath %s)" % jest_config])

_jest_test(
name = name,
data = [jest_config, ":%s_esm" % name] + deps + data,
templated_args = args,
**kwargs
)
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,10 @@
"@gregmagolan/test-a": "0.0.5",
"@types/hammerjs": "2.0.35",
"@types/jasmine": "~3.3.13",
"@types/jest": "24.9.0",
"@types/node": "^12.0.0",
"@types/semver": "6.2.0",
"babel-jest": "^25.5.1",
"bazel_workspaces": "file:./tools/npm_packages/bazel_workspaces",
"clang-format": "1.2.2",
"conventional-changelog-cli": "^2.0.21",
Expand Down
Loading

0 comments on commit b4149d8

Please sign in to comment.