Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
CooperBills opened this issue Jul 27, 2020 · 13 comments
Closed

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

CooperBills opened this issue Jul 27, 2020 · 13 comments
Assignees

Comments

@CooperBills
Copy link
Contributor

🐞 bug report

Affected Rule

The issue is caused by the rule: `rollup_bundle`

Is this a regression?

Yes, the previous version in which this bug was not present was: 1.4.0; we upgraded from 1.4.0 to 2.0.1, so the break occurred somewhere in that version range (likely with 2.0).

Description

Our rollup_bundle rules were working before upgrading to 2.0.1, but after upgrading, rollup can no longer find our imports, so the bundle generated only includes npm dependencies and the entry module. All other modules are assumed to be external dependencies and aren't included in the bundle.

🔬 Minimal Reproduction

I have a reproduction here: https://github.com/CoopeyB/rollup-bazel-bug
This reproduction is configured to fail on an unresolved import.
There is also a very hacky workaround (commented out) in the rollup.config.js file.

🔥 Exception or Error





'my_workspace/HelloWorld' is imported by bazel-out/k8-fastbuild/bin/BundleEntry.mjs, but could not be resolved – treating it as an external dependency

This error comes as a warning inside of rollup. However, if it's ignored, any dependent files outside of the initial entry of the bundle will be treated as external dependencies and not included in the bundle.

🌍 Your Environment

Operating System:

  
Linux Unbuntu 18.04.4
  

Output of bazel version:

  
3.1.0
  

Rules_nodejs version:

(Please check that you have matching versions between WORKSPACE file and @bazel/* npm packages.)

  
2.0.1
  

Anything else relevant?

This is the hacky workaround I found that manually resolves the .mjs files for the depended-on packages. However, it references bazel directories, and isn't portable:

resolveId(source) {
  const workspace = process.env.BAZEL_WORKSPACE;
  if (workspace && source.startsWith(workspace + "/")) {
    const importPath = source.substring(workspace.length + 1);
    const k8Folder = "k8-fastbuild"; // might be different on your system, e.g. "k8-opt"
    return { id: `bazel-out/${k8Folder}/bin/${importPath}.mjs`, external: false };
  }
  return null; // default resolution
}
@gregmagolan
Copy link
Collaborator

A few things missing in the minimal repro that were preventing rollup_bundle from resolving import { helloWorld } from "my_workspace/HelloWorld";

  1. rollup_bundle needs to depend on :my_workspace_pkg for the action to pick it up and for the linker to link its contents under node_modules/my_workspace so that rollup_bundle can find them there

  2. rollup.config.js needs the rollup-plugin-node-resolve plugin so that it falls back to standard node_module resolution for this import:

const node = require('rollup-plugin-node-resolve');

module.exports = {
  plugins: [
    node(),
  ],
  1. :my_workspace_pkg: itself needs to depend on whatever outputs that should be resolvable under my_workspace. This also requires an additional filegroup in the middle to pull out the .mjs outputs out of ts_library as its defaults outputs are just .d.ts files. The chain looks like this:
ts_library(
    name = "hello_world",
    srcs = ["HelloWorld.ts"],
)

# ts_library produces `.d.ts` in its default output so we require this filegroup
# to request the `es6_sources` output_group group which contains the esm `.mjs`
# files produced by ts_library. We pass this to `:my_workspace_pkg` `pkg_npm` below
# which has a `package_name` `my_workspace` so that files can be imported under
# `my_workspace/`.
filegroup(
    name = "hello_world_js",
    srcs = [":hello_world"],
    output_group = "es6_sources",
)

pkg_npm(
    name = "my_workspace_pkg",
    package_name = "my_workspace",
    deps = [
        ":hello_world_js",
    ]
)

# Include `:my_workspace_pkg` so that `rollup_bundle` can resolve
# `my_workspace/HelloWorld`.
rollup_bundle(
    name = "bundle",
    config_file = ":rollup.config.js",
    entry_point = ":BundleEntry.ts",
    deps = [
        ":bundle_entry",
        ":my_workspace_pkg",
        "@npm//rollup-plugin-node-resolve",
    ],
)

Here is the full diff to get it building:

rollup-bazel-bug (master) $ bazel build ...
INFO: Invocation ID: 0b14de1e-0555-473a-a906-3691d6cc5c2c
INFO: Options provided by the client:
  Inherited 'common' options: --isatty=1 --terminal_columns=177
INFO: Reading rc options for 'build' from /Users/greg/.bazelrc:
  Inherited 'common' options: --announce_rc
INFO: Reading rc options for 'build' from /Users/greg/oss/rollup-bazel-bug/.bazelrc:
  'build' options: --disk_cache=~/.cache/bazel-disk-cache --symlink_prefix=dist/ --nolegacy_external_runfiles --incompatible_strict_action_env
INFO: Analyzed 5 targets (0 packages loaded, 0 targets configured).
INFO: Found 5 targets...
INFO: Elapsed time: 0.104s, Critical Path: 0.00s
INFO: 0 processes.
INFO: Build completed successfully, 1 total action

rollup-bazel-bug (master) $ git diff
diff --git a/BUILD.bazel b/BUILD.bazel
index 7ccd5f6..e5ebce9 100644
--- a/BUILD.bazel
+++ b/BUILD.bazel
@@ -17,6 +17,17 @@ ts_library(
     srcs = ["HelloWorld.ts"],
 )
 
+# ts_library produces `.d.ts` in its default output so we require this filegroup
+# to request the `es6_sources` output_group group which contains the esm `.mjs`
+# files produced by ts_library. We pass this to `:my_workspace_pkg` `pkg_npm` below
+# which has a `package_name` `my_workspace` so that files can be imported under
+# `my_workspace/`.
+filegroup(
+    name = "hello_world_js",
+    srcs = [":hello_world"],
+    output_group = "es6_sources",
+)
+
 ts_library(
     name = "bundle_entry",
     srcs = ["BundleEntry.ts"],
@@ -25,16 +36,23 @@ ts_library(
     ],
 )
 
+# Include `:my_workspace_pkg` so that `rollup_bundle` can resolve
+# `my_workspace/HelloWorld`.
 rollup_bundle(
     name = "bundle",
     config_file = ":rollup.config.js",
     entry_point = ":BundleEntry.ts",
     deps = [
         ":bundle_entry",
+        ":my_workspace_pkg",
+        "@npm//rollup-plugin-node-resolve",
     ],
 )
 
 pkg_npm(
     name = "my_workspace_pkg",
     package_name = "my_workspace",
+    deps = [
+        ":hello_world_js",
+    ]
 )
diff --git a/package.json b/package.json
index ffc287b..1ce5a43 100644
--- a/package.json
+++ b/package.json
@@ -9,7 +9,8 @@
         "@bazel/rollup": "2.0.1",
         "@bazel/typescript": "2.0.1",
         "rollup": "2.23.0",
-        "typescript": "3.9.7"
+        "typescript": "3.9.7",
+        "rollup-plugin-node-resolve": "5.2.0"
     },
     "scripts": {
         "build": "bazel build //...",
diff --git a/rollup.config.js b/rollup.config.js
index 728b014..bffebe4 100644
--- a/rollup.config.js
+++ b/rollup.config.js
@@ -1,4 +1,9 @@
+const node = require('rollup-plugin-node-resolve');
+
 module.exports = {
+  plugins: [
+    node(),
+  ],
   onwarn: (warning) => {
     if (warning.code === "UNRESOLVED_IMPORT") {
       // We don't want any unresloved imports in our bundle

@purkhusid
Copy link
Contributor

@gregmagolan

This macro worked in pre 2.0.0

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"
    ts_library(
        name = "%s_ts" % name,
        srcs = srcs,
        deps = deps + ["@npm//@types/jest"],
        data = data,
    )
    native.filegroup(
        name = "%s_es5" % name,
        srcs = [":%s_ts" % name],
        output_group = "es5_sources",
    )
    args = [
        "--no-cache",
        "--no-watchman",
        "--ci",
    ]
    args.extend(["--config", "$(rootpath %s)" % jest_config])
    for src in srcs:
        args.extend(["--runTestsByPath", "$(rootpath :%s_es5)" % name])
    _jest_test(
        name = name,
        data = [jest_config, ":%s_es5" % name] + deps + data,
        args = args,
        **kwargs
    )

Do the same changes you mentioned also apply for this macro?

@matthewjh
Copy link
Contributor

matthewjh commented Aug 17, 2020

@gregmagolan there is more information in #2086 which may be useful.

While what you suggested works for simple cases, the problem is that it is a fundamental change in behaviour from what is documented, and inconsistent with ts_devserver.

Here's what the docs say:

Bazel’s TypeScript compiler has your workspace path mapped, so you can import from an absolute path starting from your workspace.

/WORKSPACE:

workspace(name = "myworkspace")
/some/long/path/to/deeply/nested/subdirectory.ts:

import {thing} from 'myworkspace/place';
will import from /place.ts.

Since this is an extension to the vanilla TypeScript compiler, editors which use the TypeScript language services to provide code completion and inline type checking will not be able to resolve the modules. In the above example, adding

"paths": {
    "myworkspace/*": ["*"]
}
to tsconfig.json will fix the imports for the common case of using absolute paths. See path mapping for more details on the paths syntax.

Similarly, you can use path mapping to teach the editor how to resolve imports from ts_library rules which set the module_name attribute.

Note it refers to this as a "mapping", i.e. it is sugar. This means that the workspace mapping is simply an alternative way to refer to modules in the repository, absolutely - from the workspace root- rather than relatively.

In the file my_workspace/a/b/my_file.ts

import A from "my_workspace/a/b/c";

should refer to the same module and binding as

import A from "./c";

This means that the following should log true in examples/angular:

import {MaterialModule} from '../shared/material/material.module';
import {MaterialModule as MaterialModule2} from 'examples_angular/src/shared/material/material.module';
console.log(MaterialModule===MaterialModule2);

And with ts_devserver, it does. But with pkg_npm and rollup_bundle, the behaviour is completely different. For one, the === will be false, because the relative import is specifying a completely distinct module to examples_angular, the latter being a "copy" under node_modules.

Not to mention that, for those of us who are using workspace references in a large monorepo extensively, it will be incredibly laborious to set up pkg_npm with the correct sub-structure. It also seems unnecessary if what we ultimately need (and what is documented) is really a path workspace-wide alias / mapping.

pkg_npm doesn't seem like the right fit here.

@matthewjh
Copy link
Contributor

matthewjh commented Aug 17, 2020

@gregmagolan

here is a repro if it helps:

matthewjh@a1736e8

  1. Run

npx bazel run //src:devserver

  1. Open in browser

  2. Observe that
    Hello World
    and
    true are logged.

These are from

src\shared\material\material.module.ts

import {NgModule} from '@angular/core';
import {MatButtonModule} from '@angular/material/button';
import {MatCardModule} from '@angular/material/card';
import {MatFormFieldModule} from '@angular/material/form-field';
import {MatGridListModule} from '@angular/material/grid-list';
import {MatIconModule} from '@angular/material/icon';
import {MatInputModule} from '@angular/material/input';
import {MatListModule} from '@angular/material/list';
import {MatMenuModule} from '@angular/material/menu';
import {MatPaginatorModule} from '@angular/material/paginator';
import {MatRadioModule} from '@angular/material/radio';
import {MatSelectModule} from '@angular/material/select';
import {MatSidenavModule} from '@angular/material/sidenav';
import {MatTableModule} from '@angular/material/table';
import {MatToolbarModule} from '@angular/material/toolbar';
import {MatTooltipModule} from '@angular/material/tooltip';
import {msg} from "./repro/msg";

console.log(msg);

const matModules = [
  MatButtonModule, MatCardModule, MatFormFieldModule, MatIconModule, MatInputModule, MatListModule,
  MatToolbarModule, MatSidenavModule, MatRadioModule, MatSelectModule, MatGridListModule,
  MatMenuModule, MatTableModule, MatPaginatorModule, MatTooltipModule
];

@NgModule({
  imports: matModules,
  exports: matModules,
})
export class MaterialModule {
}

src\app\app.module.ts

import {NgModule} from '@angular/core';
import {BrowserModule} from '@angular/platform-browser';
import {BrowserAnimationsModule} from '@angular/platform-browser/animations';
import {StoreModule} from '@ngrx/store';

import {MaterialModule} from '../shared/material/material.module';
import {MaterialModule as MaterialModule2} from 'examples_angular/src/shared/material/material.module';
import {AppRoutingModule} from './app-routing.module';
import {AppComponent} from './app.component';
import {HomeModule} from './home/home';
import {todoReducer} from './todos/reducers/reducers';

console.log(MaterialModule===MaterialModule2);
//...
  1. Now run npx bazel run //src:prodserver

  2. Now, it will build but error in the browser with

Uncaught TypeError: Failed to resolve module specifier "examples_angular/src/shared/material/material.module". Relative references must start with either "/", "./", or "../".

This is because I have no idea how to use pkg_npm correctly to achieve the correct workspace-relative path (which seems like its own limitation / problem with the 2.0 change)... BUT (bear with me), regardless, the results would not be the same as under ts_devserver, because:

console.log(MaterialModule===MaterialModule2); would be false because they are now export bindings from distinct ES modules, one from the pkg_npm files and one from the workspace.

Furthermore, it appears to me that the relative import import {msg} from "./repro/msg"; in src\shared\material\material.module.ts will fail in the pkg_npm context because only "//src/shared/material"'s own files are copied into the folder resulting from pkg_npm. What can "./repro/msg" resolve to?

image

@CooperBills
Copy link
Contributor Author

Hi @gregmagolan,

Thanks for looking into this! I really appreciate it!

That solution may work, however, I disagree with the approach. Here's my feedback:

  1. Declaring every single transitive dependency target in our workspace's pkg_npm seems... over the top. We're talking 100's of ts_libraries in our polyglot monorepository, and only a subset would be needed for a rollup build. We're not publishing any npm packages from this repository either. We manage our TS code similar to CC, python, Go, etc. where there are targets for every 1-5 files - every language is built and managed in a similar way enabling easy reuse across projects in the monorepo, which IMHO is a primary reason for using Bazel.
  2. Approaching it this way would also invalidate the Bazel cache on any TS code change (regardless of if it is used in the rollup build, since the workspace pkg_npm would declare all workspace ts/js targets), slowing down CI and caching.
  3. This isn't how nodejs_binary, ts_devserver, or really any other [INSERT_LANG_HERE]_binary works - I shouldn't have to declare every single transitive dependency manually anywhere.
  4. My code I'm building locally in rollup_bundle isn't an npm module (and may never be), so it seems weird to need pkg_npm to begin with (for us, it's only there to declare the workspace name).

Is the issue here that rollup_bundle expects js_library equivalent targets? Perhaps the real ask here is for a ts_rollup_bundle, which could configure rollup to find the es6 sources from ts_library deps and their transitive ts_library dependencies.

Although on second thought, the mjs files are already in the bazel build sandbox, but rollup doesn't know how to resolve them. It still feels like an issue with the rollup_bundle rule and not with how the repository is configured (If rollup needs all imports declared as npm modules, that's an implementation detail that, IMHO, rollup_bundle should abstract away).

@ajbouh
Copy link

ajbouh commented Aug 18, 2020

This concern exactly matches my own thinking. Seems like we need to fix the contract / implementation of rollup_bundle?

@alexeagle
Copy link
Collaborator

You could add a resolver plugin to rollup that knows how to find the bazel-out files, but the problem described here isn't really rollup-specific - anything downstream of a tree of ts_library rules may want to resolve the files.

Seems like the right resolution is we can add back an optional flag so you can opt-out of this breaking change.

@ajbouh
Copy link

ajbouh commented Aug 18, 2020

What exactly was the breaking change that would get put back?

@alexeagle
Copy link
Collaborator

#1797

@ajbouh
Copy link

ajbouh commented Aug 27, 2020

It's hard to know for sure, but I think found a workaround to this problem.

By adding this to all of my ts_library targets:

module_name = 'workspace/package/path',
module_root = 'index.d.ts',

rollup_bundle seems to be able to find them without any extra code. This makes sense when I see the implementation of ts_library:

https://github.com/bazelbuild/rules_nodejs/blob/stable/packages/typescript/internal/build_defs.bzl#L335

    if ctx.attr.module_name:
        path = "/".join([p for p in [ctx.bin_dir.path, ctx.label.workspace_root, ctx.label.package] if p])
        ts_providers["providers"].append(LinkablePackageInfo(
            package_name = ctx.attr.module_name,
            path = path,
            files = ts_providers["typescript"]["es5_sources"],
            _tslibrary = True,
        ))

It's not great to have to hard-code the workspace name and package into every single ts_library, but it's been preferable to everything else I've tried.

Would this be a solution to the original issue? If so, perhaps there are some sane defaults we can add to ts_library?

Perhaps something like

    module_name = ctx.attr.module_name
    if ctx.attr.default_module_name and not module_name:
        module_name = "/".join([ctx.workspace_name, ctx.label.package])

    if module_name:
        path = "/".join([p for p in [ctx.bin_dir.path, ctx.label.workspace_root, ctx.label.package] if p])
        ts_providers["providers"].append(LinkablePackageInfo(
            package_name = ctx.attr.module_name,
            path = path,
            files = ts_providers["typescript"]["es5_sources"],
            _tslibrary = True,
        ))

My experiments also indicate that module_root = 'index.d.ts' makes this strategy work more often, but I'm not sure why or even if this is strictly true.

@jbedard
Copy link
Collaborator

jbedard commented Aug 27, 2020

perhaps there are some sane defaults we can add to ts_library

Basically the default module_name would be the {workspace}/{path}, where path is the folder the BUILD file is in?

@ajbouh
Copy link

ajbouh commented Aug 27, 2020

Yes, that's what I've been doing locally and what the code above suggests

@alexeagle
Copy link
Collaborator

Should be resolved by #2175 which adds back the ability for rollup_bundle to link the root of the workspace into the node_modules tree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants