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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Every new npm dep needs a unique approach how to add it to ts_devserver #1055

Closed
rayman1104 opened this issue Aug 23, 2019 · 4 comments
Closed

Comments

@rayman1104
Copy link
Collaborator

rayman1104 commented Aug 23, 2019

馃悶 bug report

Affected Rule

The issue is caused by the rule:
  • ts_devserver
  • npm_umd_bundle

Is this a regression?

No

Description

I have just migrated one of our new Angular projects to Bazel and here I want to report all the issues I had with some of npm deps (probably it is the first time those deps are imported in Bazel project, so it might be helpful for someone).
The main issue for most of them is similar to this: angular/angular-bazel-example#56 (Error: Mismatched anonymous define() module: function).
But I had to find an individual workaround for each of them (see below). I'm curious, maybe there already exists the one common approach how to build any npm dep even with unnamed UMD and I don't know it? Looks like npm_umd_bundle is a good candidate, but there are no examples how to use it and still, some modules need other tweaks (see point 1). Or does it make sense to send PR fixes to all modules that don't name their UMD bundle? Would appreciate your help.


  1. @nebular/theme, @nebular/eva-icons

Here is the repro for nebular: scalio/bazel-nx-example#4

For these deps I had to clone nebular repo and add amd.id to their rollup bundle. I've also tried several other approaches as well, such as using npm_umd_bundle rule, but unfortunately, it raises the error similar to this: #771 (worth to mention, I'm on latest rules_nodejs).

Also, after this helped, I had to fix intersection-observer and eva-icons transitive dependencies.
intersection-observer can be just added to additional_root_paths and data of ts_devserver rule.
As for eva-icons, the issue there is that they export with just eva name, so adding this UMD shim to scripts worked out:

// eva-icons
(function(factory) {
    if (typeof module === 'object' && typeof module.exports === 'object') {
        var v = factory(require, exports);
        if (v !== undefined) module.exports = v;
    } else if (typeof define === 'function' && define.amd) {
        define('eva-icons', ['exports', 'eva'], factory);
    }
})(function(exports, eva) {
    'use strict';
    Object.keys(eva).forEach(function(key) {
        exports[key] = eva[key];
    });
    Object.defineProperty(exports, '__esModule', {value: true});
});

Btw, I still didn't manage to make eva-icons work in production mode. There is some runtime error I guess with eva <-> eva-icons namings:

TypeError: Error resolving module specifier: eva-icons

  1. ng2-dragula, @ngxs

For all angular-related modules, I had to add paths to angular-metadata.tsconfig.json like in angular-bazel-example. That's easy after figuring out once that you need to do it. But maybe worth to make better documentation for it or proper error message, cause it may be tricky for the first time.

As for ng2-dragula, it has a dragula transitive dep with unnamed UMD. But this time npm_umd_bundle really helped:

npm_umd_bundle(
    name = "dragula",
    package_name = "dragula",
    entry_point = "@npm//:node_modules/dragula/dragula.js",
    package = "@npm//dragula",
)

  1. immutable, dom-autoscroller

For these deps npm_umd_bundle worked out well too:

  
npm_umd_bundle(
    name = "immutable",
    package_name = "immutable",
    entry_point = "@npm//:node_modules/immutable/dist/immutable.js",
    package = "@npm//immutable",
)

npm_umd_bundle(
    name = "dom-autoscroller",
    package_name = "dom-autoscroller",
    entry_point = "@npm//:node_modules/dom-autoscroller/dist/bundle.js",
    package = "@npm//dom-autoscroller",
)

ts_devserver(
  ...
  scripts = [
        ...
        ":dom-autoscroller",
        ":immutable",
        ":dragula",
        ...
  ]
  ...
)
  

If this rule is so helpful, maybe still worth to add documentation for it? ;)
(#951)

As for dom-autoscroller, also had to add some lines to tsconfig to fix the import error:

ERROR TypeError: "dom_autoscroller_1.default is not a function"

Here are the lines:

  "esModuleInterop": true,
  "allowSyntheticDefaultImports": true,

Related issue: #933 (comment)


馃敩 Minimal Reproduction

Here is the repro for nebular: scalio/bazel-nx-example#4

馃實 Your Environment

Operating System:

  
No LSB modules are available.
Distributor ID:	Ubuntu
Description:	Ubuntu 18.04.2 LTS
Release:	18.04
Codename:	bionic
  

Output of bazel version:

  
Build label: 0.27.0
Build target: bazel-out/k8-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Mon Jun 17 13:00:46 2019 (1560776446)
Build timestamp: 1560776446
Build timestamp as int: 1560776446
  

Rules version (SHA):

  
http_archive(
    name = "build_bazel_rules_nodejs",
    sha256 = "3356c6b767403392bab018ce91625f6d15ff8f11c6d772dc84bc9cada01c669a",
    urls = ["https://github.com/bazelbuild/rules_nodejs/releases/download/0.36.1/rules_nodejs-0.36.1.tar.gz"],
)
  
@alexeagle
Copy link
Collaborator

alexeagle commented Aug 26, 2019

Thanks for the detailed feedback! Dealing with third-party dependencies is certainly one of our rough edges.


First reply: for any package you've installed, we create a npm_umd_bundle rule in the generated BUILD file, so for example, after I install dragula I get can

$ bazel query --output=label_kind @npm//dragula:all
npm_umd_bundle rule @npm//dragula:dragula__umd
node_module_library rule @npm//dragula:dragula__typings
node_module_library rule @npm//dragula:dragula
node_module_library rule @npm//dragula:dragula__contents
filegroup rule @npm//dragula:dragula__files

see that a npm_umd_bundle is in there, so you can just use @npm//dragula:dragula__umd instead of declaring one of these yourself.


For the dom-autoscroller error, the tsconfig.json is under the user's control, so I don't think there's anything to change in rules_nodejs to account for this.


For the failure to produce a npm_umd_bundle for @nebula/theme, I'm trying to reproduce this, I get error

Error: Cannot find module '@angular/forms' from '/usr/local/google/home/alexeagle/.cache/bazel/_bazel_alexeagle/c410d6070ef2d359d26ac4aeb886e1ea/sandbox/linux-sandbox/3/execroot/build_bazel_rules_nodejs/external/npm/node_modules/@nebular/theme/bundles'

even though I do have @angular/forms installed. Is that the same error you see?

@rayman1104
Copy link
Collaborator Author

rayman1104 commented Aug 27, 2019

Ah right, cool! Umd targets can be just reused and now it looks like this:

ts_devserver(
  ...
  scripts = [
        ...
        "@npm//immutable:immutable__umd",
        "@npm//dom-autoscroller:dom-autoscroller__umd",
        "@npm//dragula:dragula__umd",
        ...
  ]
  ...
)

As for nebular, yes they forgot to add @angular/forms to peerDependencies, but that wasn't a big issue for me (I don't know why, but I have it installed in node_modules and it worked out).

So, I've just created 2 patched versions of nebular fork for more convenience. Sources are here: https://github.com/scalio/nebular/commits/fix/bazel

4.2.0-patch is fix for @angular/forms peerDependency;
4.2.0-patch2 also fixes Error: Mismatched anonymous define() module: function.

You can add them to package.json, if you want. Like this:

    "@nebular/eva-icons": "npm:@scalio-oss/nebular-eva-icons@4.2.0-patch",
    "@nebular/theme": "npm:@scalio-oss/nebular-theme@4.2.0-patch",

I'm still curious if we need to send them that as PR or it can be fixed just via Bazel rules?
Also would appreciate your help to resolve a production error with eva-icons in web_package.

@alexeagle
Copy link
Collaborator

We've historically sent PRs to get projects to add module names into their UMD boilerplate, but of course it's a never-ending task to try to convert the whole ecosystem so it doesn't scale. We should really make ng-packagr produce named UMD outputs, then as the ecosystem upgrades their Angular CLI they'll get the packaging fix - but that is still just a subset of packages we need to handle.

We just need to accept all the module formats and either do a trick on them at install time to provide a label that does the conversion (and make our bundling rules automatically select the converted modules) or teach the ts_devserver (concatjs really) to do module transpilation. Or recommend a more common devserver that knows how to do it (and keeps our scalability guarantee)
Anyway that's a longer-term project, you're doing the right thing for now.

Could you file a separate issue for why eva-icons doesn't work after the rollup bundling? This one is about ts_devserver.

@rayman1104
Copy link
Collaborator Author

Gottcha, thanks! It becomes clearer now.
Closing this issue then and will file others soon.

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

2 participants