rollup_bundle should use write_amd_names_shim

[erickj]

Hi all,

Thanks for this package, it's coming along nicely. What are the plans for supporting commonjs style dependencies in rollup bundles? In rollup_bundle.bzl I see that there is currently an unused TMPL_additional_plugins, however I'm unsure of the planned format of this, given that in the rollup.config.js is simply expecting the contents to either be previously required, or a full require+call expression (e.g. require('myplugin')())?

https://github.com/bazelbuild/rules_nodejs/blob/master/internal/rollup/rollup.config.js#L108

In any case, the rollup-plugin-commonjs is common enough that it probably warrants inclusion in the base set of plugins by default (bazelResolve, nodeResolve, and commonjsResolve).

I've hacked in default support for commonjsResolve into my own fork, however it was done w/o any thought to integrating it with the rollup_bundle rule give it's only for my use case.

erickj@d4c8322

What's the future direction here w/ either actually implementing TMPL_additional_plugins or first party support for the cjs plugin?

[alexeagle]

Angular uses that undocumented TMPL_additional_plugins from ng_rollup_bundle: https://github.com/angular/angular/blob/master/packages/bazel/src/ng_rollup_bundle.bzl#L27
The intent is that Bazel rules can extend rollup_bundle this way, but not Bazel BUILD files (at least not yet).

The commonjs plugin causes problems, for example it tries to load rxjs commonjs distro which doesn't tree-shake well. We'd need the right assertions in tests to prevent the commonjs plugin loading code that was built by bazel.

Can you share the use case? I assume you'd like to bundle some off-the-shelf JS library with your app?

[erickj]

"for example it tries to load rxjs commonjs distro which doesn't tree-shake well"

:/ i've noticed

"I assume you'd like to bundle some off-the-shelf JS library with your app"

exactly, specifically protobufjs at the moment. there are several other libs as well. Unfortunately cjs is just unavoidable ATM, practically speaking.

[erickj]

One update re:

"The commonjs plugin causes problems, for example it tries to load rxjs commonjs distro which doesn't tree-shake well."

IIUC that's actually the fault of the rxjs package.json file previously only declaring pkg.main to point to the root index.js file, which is a cjs module. Now that rxjs is properly using pkg.module to point to the _esm5 targets plus per subpackage package.json files this should be fixed and a non-issue for including rollup-commonjs.

It appears then that the real fix is for package maintainers to migrate towards explicit es6 module support. In the meanwhile, providing cjs support by default (or at least optionally) seems reasonable.

[Globegitter]

I think it would be great in general if it was possible to have an easy way to provide further rollup plugins. Just testing this out and we are also in the need of the json plugin, the babel plugin, etc.

But thanks @alexeagle for the pointer on how Angular does it currently. That should more than do for the time being.

[alexeagle]

@Globegitter someone has proposed a babel_library rule that would be similar to ts_library. What would you use the json plugin for? Remember under Bazel, we want to avoid Rollup expanding to become a build tool - it's not incremental and not idiomatic. Bazel should run all the different tools as different processes. Making build-optimizer into a Rollup plugin was a workaround for wrong layering in the Angular toolchain.

Back to the primary thread here - I added an amd_names attribute in js_library
https://github.com/bazelbuild/rules_nodejs/blob/master/internal/js_library/js_library.bzl#L62
that we use in ts_devserver and ts_web_test to load the runtime protobufjs support. I think we just need to wire this up the same way for rollup_bundle

[Globegitter]

@alexeagle yeah you are right about that the json plugin is for a bazel non-idiomatic usage. This comment is also related #149 (comment)

[alexeagle]

@gregmagolan merged a change to the rollup_bundle rule yesterday to error if there is an unresolved external module. Thinking more, I was probably wrong in my last comment - this should already work with the write_amd_names_shim. The idea here is that a combination of the UMD distro of the library (which exposes a global variable) plus a define call to give an AMD module name to that global variable, is sufficient to make this work in the bundler without any CJS loading.
Greg, we should discuss this issue, I'm not clear what we need to do here.

[gregmagolan]

Here's what I ended up with to get the protobuf example in rules_typescript to bundle with rollup: https://github.com/gregmagolan/rules_typescript/blob/e771ee7f29868a3ab0e763afbf1b161d68606cdd/examples/protocol_buffers/BUILD.bazel#L64. Rollup treats the protobuf imports as globals and the protobuf code needs to be loaded outside of the bundle in a script tag. Will think about this some more about how to make it simpler but this is one way to do it.

The rest of the diff is here: https://github.com/bazelbuild/rules_typescript/pull/204/files#diff-3db4baf0fd0becb125c0e7860df3637eR64

[alexeagle]

@gregmagolan can we close this one?

[alexeagle]

I think this can be closed now that the new rollup_bundle rule gives you control of the plugins