Skip to content

Commit

Permalink
fix(builtin): support for scoped modules in linker
Browse files Browse the repository at this point in the history
  • Loading branch information
gregmagolan authored and alexeagle committed Sep 26, 2019
1 parent 051b592 commit c568583
Show file tree
Hide file tree
Showing 9 changed files with 85 additions and 11 deletions.
28 changes: 23 additions & 5 deletions internal/linker/index.js

Large diffs are not rendered by default.

27 changes: 23 additions & 4 deletions internal/linker/link_node_modules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,17 @@ Include as much of the build output as you can without disclosing anything confi
`);
}

/**
* Create a new directory and any necessary subdirectories
* if they do not exist.
*/
function mkdirp(p: string) {
if (!fs.existsSync(p)) {
mkdirp(path.dirname(p));
fs.mkdirSync(p);
}
}

async function symlink(target: string, path: string) {
log_verbose(`symlink( ${path} -> ${target} )`);
// Use junction on Windows since symlinks require elevated permissions.
Expand All @@ -46,7 +57,7 @@ async function symlink(target: string, path: string) {
if (!fs.existsSync(path)) {
log_verbose(
'ERROR\n***\nLooks like we created a bad symlink:' +
`\n pwd ${process.cwd()}\n target ${target}\n***`);
`\n pwd ${process.cwd()}\n target ${target}\n path ${path}\n***`);
}
}
}
Expand Down Expand Up @@ -262,7 +273,7 @@ export async function main(args: string[], runfiles: Runfiles) {
process.chdir(rootDir);

// Symlinks to packages need to reach back to the workspace/runfiles directory
const workspaceRelative = path.relative('.', workspaceDir);
const workspaceAbs = path.resolve(workspaceDir);

// Now add symlinks to each of our first-party packages so they appear under the node_modules tree
const links = [];
Expand All @@ -273,7 +284,7 @@ export async function main(args: string[], runfiles: Runfiles) {
switch (root) {
case 'bin':
// FIXME(#1196)
target = path.join(workspaceRelative, bin, toWorkspaceDir(modulePath));
target = path.join(workspaceAbs, bin, toWorkspaceDir(modulePath));
// Spend an extra FS lookup to give better error in this case
if (!await exists(target)) {
// TODO: there should be some docs explaining how users are
Expand All @@ -287,7 +298,7 @@ export async function main(args: string[], runfiles: Runfiles) {
}
break;
case 'src':
target = path.join(workspaceRelative, toWorkspaceDir(modulePath));
target = path.join(workspaceAbs, toWorkspaceDir(modulePath));
break;
case 'runfiles':
target = runfiles.resolve(modulePath) || '<runfiles resolution failed>';
Expand All @@ -298,6 +309,14 @@ export async function main(args: string[], runfiles: Runfiles) {
}

for (const m of Object.keys(modules)) {
const segments = m.split('/');
if (segments.length > 2) {
throw new Error(`module ${m} has more than 2 segments which is not a valid node module name`);
}
if (segments.length == 2) {
// ensure the scope exists
mkdirp(segments[0]);
}
const [kind, modulePath] = modules[m];
links.push(linkModule(m, kind, modulePath));
}
Expand Down
2 changes: 2 additions & 0 deletions internal/linker/test/integration/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ sh_binary(
":program.js",
"//internal/linker:index.js",
"//internal/linker/test/integration/static_linked_pkg",
"//internal/linker/test/integration/static_linked_scoped_pkg",
"@bazel_tools//tools/bash/runfiles",
"@build_bazel_rules_nodejs//toolchains/node:node_bin",
],
Expand All @@ -35,6 +36,7 @@ linked(
"//%s/absolute_import:index.js" % package_name(),
":run_program",
"//internal/linker/test/integration/dynamic_linked_pkg",
"//internal/linker/test/integration/dynamic_linked_scoped_pkg",
"@npm//semver",
],
)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
load("//internal/js_library:js_library.bzl", "js_library")

package(default_visibility = ["//internal/linker/test:__subpackages__"])

js_library(
name = "dynamic_linked_scoped_pkg",
srcs = ["index.js"],
module_from_src = True,
module_name = "@linker_scoped/dynamic_linked",
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
function addD(str) {
return `${str}_d`;
}

exports.addD = addD;
2 changes: 1 addition & 1 deletion internal/linker/test/integration/golden.txt
Original file line number Diff line number Diff line change
@@ -1 +1 @@
1.2.3_c_b_a
1.2.3_a_b_c_d_e
8 changes: 7 additions & 1 deletion internal/linker/test/integration/program.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,15 @@ const a = require('static_linked');
const b = require('dynamic_linked');
// We've always supported `require('my_workspace')` for absolute imports like Google does it
const c = require('build_bazel_rules_nodejs/internal/linker/test/integration/absolute_import');
// First-party package from ./static_linked_scoped_pkg
// it should get resolved through runfiles
const d = require('@linker_scoped/dynamic_linked');
// First-party package from ./dynamic_linked_scoped_pkg
// it should get resolved from the execroot
const e = require('@linker_scoped/static_linked');

// Third-party package installed in the root node_modules
const semver = require('semver');

// This output should match what's in the golden.txt file
console.log(a.addA(b.addB(c.addC(semver.clean(' =v1.2.3 ')))));
console.log(e.addE(d.addD(c.addC(b.addB(a.addA(semver.clean(' =v1.2.3 ')))))));
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
load("//internal/js_library:js_library.bzl", "js_library")

package(default_visibility = ["//internal/linker/test:__subpackages__"])

js_library(
name = "static_linked_scoped_pkg",
srcs = ["index.js"],
module_name = "@linker_scoped/static_linked",
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
function addE(str) {
return `${str}_e`;
}

exports.addE = addE;

0 comments on commit c568583

Please sign in to comment.