fix(builtin): linker no longer makes node_modules symlink to the root of the workspace output tree #1797

gregmagolan · 2020-04-06T20:43:30Z

For 2.0.

Thanks @mrmeku for updating ts_proto_library for this PR.

BREAKING CHANGES:

Any nodejs_binary/nodejs_test processes with the linker enabled (--nobazel_patch_module_resolver is set) that were relying on standard node_module resolution to resolve manifest file paths such as my_workspace/path/to/output/file.js must now use the runfiles helper such as.

Previously:

const absPath = require.resolve('my_workspace/path/to/output/file.js');

With runfiles helper:

const runfiles = require(process.env['BAZEL_NODE_RUNFILES_HELPER']);
const absPath = runfiles.resolve('my_workspace/path/to/output/file.js');

Angular ViewEngine + Bazel is no longer supported in rules_nodejs 2.0 as the generated ngfactory & ngsummary files depend on absolute imports. Users upgrading to rules_nodejs 2.0 will also need to upgrade to Ivy if they are on ViewEngine.

gregmagolan · 2020-04-10T15:08:16Z

Pretty much done except for issues with the ts_proto_library labs rule which has examples & tests that depends on absolute imports. @mrmeku PTAL.

googlebot · 2020-04-16T20:29:22Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2020-04-17T21:13:14Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

alexeagle · 2020-04-20T17:39:52Z

What do we do about the recommendation in a monorepo that you can avoid crazy import {} from '../../../../../../path/to/thing' and instead always import {} from my_wksp/path/to/thing
do we have any revised guidance? is there a README somewhere that needs to be updated?

alexeagle

I think this is deleting the sources for bazel.angular.io such that we can never deploy it again. Not sure if that's a problem but we should coordinate with @IgorMinar about tearing that down

alexeagle

I think ts_library pathmapping is still going to allow code to type-check with these workspace-absolute paths, is that a problem that it type-checks but can't run?

alexeagle · 2020-04-20T17:47:21Z

examples/angular_view_engine/src/index.html

-              </li>
-              <li>
-                <a
-                  href="https://medium.com/@Jakeherringbone/running-tools-under-bazel-8aa416e7090c"


all this content we didn't really mean to delete...

alexeagle · 2020-04-20T17:48:52Z

internal/linker/test/integration/program.js

+
+let c;
+try {
+  // As of 2.0, we no longer support `require('my_workspace/path/to/output/file.js')` for absolute


would be nice to link to this spot in the BREAKING CHANGE

IgorMinar · 2020-04-21T14:00:13Z

Let's coordinate. I have a meeting with Greg later this week so we can cover this as well.

@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

…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.

@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 · 2020-06-09T19:14:48Z

@gregmagolan if you don't have time to re-base and land, we could hand to someone else?

alexeagle · 2020-06-12T20:25:33Z

Need to check that it's still possible for users to link their project root by putting a pkg_npm or something in the /BUILD.bazel

… of the workspace output tree BREAKING CHANGE: Any nodejs_binary/nodejs_test processes with the linker enabled (--nobazel_patch_module_resolver is set) that were relying on standard node_module resolution to resolve manfest file paths such as `my_workspace/path/to/output/file.js` must now use the runfiles helper such as. Previously: ``` const absPath = require.resolve('my_workspace/path/to/output/file.js'); ``` With runfiles helper: ``` const runfiles = require(process.env['BAZEL_NODE_RUNFILES_HELPER']); const absPath = runfiles.resolve('my_workspace/path/to/output/file.js'); ```

…kspace symlink Absolute imports no longer supported via require. You must use the runfiles helper.

… symlink

…pace symlink

…ript/test

alexeagle · 2020-06-13T19:54:26Z

Windows failure FYI


Test output for //packages/typescript/test/ts_library_esm_with_jest:test:
--
  | FAIL ../../lib.test.js
  | ● Test suite failed to run
  |  
  | Cannot find module 'build_bazel_rules_nodejs/packages/typescript/test/ts_library_esm_with_jest/lib' from 'lib.test.js'
  |  
  | > 1 \| import {doStuff} from './lib';
  | \| ^
  | 2 \|
  | 3 \| describe('doStuff', () => {
  | 4 \|   it('should do some stuff', () => {
  |  
  | at Resolver.resolveModule (../../../../../../../node_modules/jest-resolve/build/index.js:296:11)
  | at ../../../../../../../packages/typescript/test/ts_library_esm_with_jest/lib.test.ts:1:1
  | at lib.test.js:3:17
  | at Object.<anonymous> (lib.test.js:9:3)

alexeagle

looks like angular_view_engine is the only example that shows how to put a pkg_npm in the root to get workspace-absolute imports? Maybe we should do this in some other example or a new one, so if we delete angular_view_engine at some point, we aren't missing one

alexeagle · 2020-06-13T19:55:30Z

examples/protocol_buffers/BUILD.bazel

@@ -5,6 +5,7 @@ load("@npm//@bazel/rollup:index.bzl", "rollup_bundle")
 load("@npm//@bazel/terser:index.bzl", "terser_minified")
 load("@npm//@bazel/typescript:index.bzl", "ts_config", "ts_devserver", "ts_library")
 load("@npm//http-server:index.bzl", "http_server")
+load("@rules_proto//proto:defs.bzl", "proto_library")


was this just a warning you're fixing or is it related to the change?

🤔 where did that symbol come from before I added that load?

…examples/angular_view_engine

alexeagle · 2020-09-10T14:40:41Z

Note, #2175 added back the ability to link the workspace root into the node_modules tree

gregmagolan added the package:builtin label Apr 6, 2020

gregmagolan added this to the 2.0 milestone Apr 6, 2020

gregmagolan requested review from alexeagle and soldair as code owners April 6, 2020 20:43

googlebot added the cla: yes label Apr 6, 2020

gregmagolan force-pushed the linker_no_root_symlink branch 2 times, most recently from 2ad92b2 to b3b9f0e Compare April 8, 2020 01:36

gregmagolan changed the title ~~fix(builtin): linker no longer makes node_modules symlink to the rootof the workspace output tree~~ WIP fix(builtin): linker no longer makes node_modules symlink to the rootof the workspace output tree Apr 8, 2020

alexeagle mentioned this pull request Apr 9, 2020

Missing @npm/:node_modules/ input files because repo rule isn't being triggered #1701

Closed

gregmagolan force-pushed the linker_no_root_symlink branch 2 times, most recently from b8e4def to f83daca Compare April 10, 2020 14:20

googlebot added cla: no and removed cla: yes labels Apr 16, 2020

gregmagolan force-pushed the linker_no_root_symlink branch from 78af717 to a55a2c6 Compare April 17, 2020 21:12

googlebot added cla: yes and removed cla: no labels Apr 17, 2020

gregmagolan changed the title ~~WIP fix(builtin): linker no longer makes node_modules symlink to the rootof the workspace output tree~~ fix(builtin): linker no longer makes node_modules symlink to the root of the workspace output tree Apr 17, 2020

gregmagolan force-pushed the linker_no_root_symlink branch from 9a13dfc to 72fcc04 Compare April 17, 2020 21:30

alexeagle reviewed Apr 20, 2020

View reviewed changes

gregmagolan force-pushed the linker_no_root_symlink branch from 72fcc04 to 8f1e084 Compare April 22, 2020 21:20

gregmagolan mentioned this pull request Apr 29, 2020

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

Merged

12 tasks

gregmagolan force-pushed the linker_no_root_symlink branch from 8f1e084 to 95c518a Compare June 13, 2020 01:12

gregmagolan requested a review from mattem as a code owner June 13, 2020 01:12

gregmagolan added 4 commits June 12, 2020 18:13

test(builtin): update linker integration tests to reflect removed wor…

ff47e3b

…kspace symlink Absolute imports no longer supported via require. You must use the runfiles helper.

refactor(labs): updated grpc_web imports to reflect removed workspace…

11d1723

… symlink

refactor(examples): updated grpc_web imports to reflect removed works…

3742a9f

…pace symlink

gregmagolan force-pushed the linker_no_root_symlink branch 3 times, most recently from c5a749b to 3742a9f Compare June 13, 2020 01:31

gregmagolan added 3 commits June 12, 2020 18:42

chore: fixes after rebase

b56a530

chore: remove test that no longer applies after this change

eb42481

chore: move ts_library, esm, jest integration test to packages/typesc…

4d1b1b7

…ript/test

alexeagle approved these changes Jun 13, 2020

View reviewed changes

alexeagle force-pushed the linker_no_root_symlink branch 4 times, most recently from 557d7fc to 9faf03f Compare June 16, 2020 05:55

refactor(examples): use pkg_npm to replace root workspace linking in …

d65ec15

…examples/angular_view_engine

alexeagle force-pushed the linker_no_root_symlink branch from 9faf03f to d65ec15 Compare June 16, 2020 06:01

alexeagle merged commit 080b460 into bazel-contrib:master Jun 16, 2020

alexeagle mentioned this pull request Aug 22, 2020

rollup_bundle cannot resolve imports (rules nodejs 2.0+) #2083

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(builtin): linker no longer makes node_modules symlink to the root of the workspace output tree #1797

fix(builtin): linker no longer makes node_modules symlink to the root of the workspace output tree #1797

gregmagolan commented Apr 6, 2020 •

edited

Loading

gregmagolan commented Apr 10, 2020

googlebot commented Apr 16, 2020

googlebot commented Apr 17, 2020

alexeagle commented Apr 20, 2020

alexeagle left a comment

alexeagle left a comment

alexeagle Apr 20, 2020

alexeagle Apr 20, 2020

IgorMinar commented Apr 21, 2020

alexeagle commented Jun 9, 2020

alexeagle commented Jun 12, 2020

alexeagle commented Jun 13, 2020

alexeagle left a comment

alexeagle Jun 13, 2020

gregmagolan Jun 16, 2020

alexeagle commented Sep 10, 2020

fix(builtin): linker no longer makes node_modules symlink to the root of the workspace output tree #1797

fix(builtin): linker no longer makes node_modules symlink to the root of the workspace output tree #1797

Conversation

gregmagolan commented Apr 6, 2020 • edited Loading

gregmagolan commented Apr 10, 2020

googlebot commented Apr 16, 2020

googlebot commented Apr 17, 2020

alexeagle commented Apr 20, 2020

alexeagle left a comment

Choose a reason for hiding this comment

alexeagle left a comment

Choose a reason for hiding this comment

alexeagle Apr 20, 2020

Choose a reason for hiding this comment

alexeagle Apr 20, 2020

Choose a reason for hiding this comment

IgorMinar commented Apr 21, 2020

alexeagle commented Jun 9, 2020

alexeagle commented Jun 12, 2020

alexeagle commented Jun 13, 2020

alexeagle left a comment

Choose a reason for hiding this comment

alexeagle Jun 13, 2020

Choose a reason for hiding this comment

gregmagolan Jun 16, 2020

Choose a reason for hiding this comment

alexeagle commented Sep 10, 2020

gregmagolan commented Apr 6, 2020 •

edited

Loading