bazel-contrib / rules_nodejs

NodeJS toolchain for Bazel.
https://bazelbuild.github.io/rules_nodejs/
Apache License 2.0
725 stars 522 forks source link

Infer `module_name` and `module_root` from providers. #278

Closed hsyed closed 4 years ago

hsyed commented 6 years ago

Kotlin rules will have JS support soon. The node rules are currently inferring the module_name and module_root attributes from the attributes.

The module_root will always be label + ".js" in kotlin and module_name currently has to be the same as the label as the kotlinc-js compiler infers the module name from the jar.

So rules nodejs should also look at the providers for module_name and module_root. Should we use a proper provider here (a provider exported from these rules) or a legacy provider ?

To be clear I mean the following by a legacy provider:

...
   return struct(
       nodejs = struct(
           module_name = ctx.label.name,
           module_root = ctx.label.name + ".js"
       )
       providers=[default_info, kt_info] #these are created earlier on in the rule impl
   )
alexeagle commented 6 years ago

Sorry, this isn't quite clear to me. What's the shape of the graph? A nodejs_binary depends on a kt_library and so it should iterate its deps and check if they offer some provider?

Also, why does the module name get set in this way? Is that because Kotlin compiler produces commonjs require statements that make some assumptions about how the modules will be named?

hsyed commented 6 years ago

Sorry, this isn't quite clear to me. What's the shape of the graph? A nodejs_binary depends on a kt_library and so it should iterate its deps and check if they offer some provider?

Yes, this would mean the module_name and module_root would first be checked in a provider before being checked for as a attr. This would let the rule implementation decide on what they should be set to and they don't need to be documented as attrs.

Also, why does the module name get set in this way? Is that because Kotlin compiler produces commonjs require statements that make some assumptions about how the modules will be named?

Yes the Kotlin2-js compiler sees a acme-auth.jar in it's library list and will generate javascript with a require("acme-auth") in it.

I toyed around with changing the output to use the module_name in the outputs (rather than name):

    outputs = dict(
        js = "%{module_name}.js",
        js_map = "%{module_name}.js.map",
        jar = "%{module_name}.jar",
        srcjar = "%{module_name}-sources.jar",
    ),

This just felt very unidiomatic.

alexeagle commented 6 years ago

Okay I tried out your node example in rules_kotlin and looked in bazel-bin to understand a bit better how the parts are assembled.

I think you're just suggesting that you could remove this code https://github.com/bazelbuild/rules_kotlin/blob/643b3e5a7064dde1c4a3a9feb274575029ad60e8/kotlin/internal/js/js.bzl#L37-L48

if we changed https://github.com/bazelbuild/rules_nodejs/blob/4ef01cfa45018f7e1a7cbf27aac1cc2760d8447f/internal/common/module_mappings.bzl#L62 to also check a provider on each rule we visit?

But maybe another answer is just for your kt_js_import_macro to just wrap the output .js files in a js_library https://github.com/bazelbuild/rules_nodejs/blob/4ef01cfa45018f7e1a7cbf27aac1cc2760d8447f/internal/js_library/js_library.bzl#L59 that provides the module name, then none of your rules needs a module_name/module_root attribute.

hsyed commented 6 years ago

Yes you got it.

I did have a look at js_library but a sandwich is not feasible in this case because the kt_js_library also ferries along a Jar containing bytecode and a source-jar. These are needed as deps for other kotlin_js_libraries and the intellij plugin also uses these for type information.

What about the following provider ?

# Or NodeJsInfo
JsModuleInfo = provider(
    fields = {
         # in the case of Kotlin this is always a single file, not minified though.
        "runfiles": "depset of file. Will contain .js files, .js.map files and any other files needed at runtime. At least a single js file has to be present", 
        "runtime_deps": "depset of NodeModuleInfo, optional. The transitive dependencies",
        "module_name": "str. The module name",
        # This might be better as a file ? 
        "module_root": "str: The module root", 
    }
)

With the runtime_deps above the other issue could also be closed I think. Or at least it won't strictly be needed for Kotlin as the kotlin-stdlib would always be in the runtime_deps.


On a side note with this approach or what you suggested there would be a dependency between kotlin_rules for js support and node_rules but I think this is unavoidable.

There are some internal macros / action macros that I could conceivably make use of in the kotlin rules (the rollup actions for example). I can see they have been written for reuse, would be good to have some documentation as to weather they may be reused in other rules.

hsyed commented 6 years ago

BTW I updated the kt_js_import yesterday to unpack the jar to pull out the js files so that the stdlibs don't need to be declared in package.json. So the macro is used in two places now.

kotlin stdlib breaks the naming conventions for the jar names -- the module name of the jar kotlin-stdlib-js is kotlin and kotlin-test-js is kotlin-test. I think the js suffix is part of a convention for multiplatform modules.

alexeagle commented 6 years ago

I think we need a Provider symbol exported by rules_nodejs that is expected from rules in the deps, and which transports the needed info. Then you need to export this provider in order to work with nodejs rules. TypeScript already needs this, there's a TODO in this repo to stop special-casing the typescript provider in jasmine_node_test for example. I think we could do this in a couple weeks at best, currently prototyping some other substantial changes.

nimerritt commented 6 years ago

@alexeagle I'm also interested in this. We are using higher-level rules to ensure we track external NPM dependencies at a fine enough level that for each "module" we can generate a package.json file with the dependency section automatically generated. Currently I'm using macros and wrap outputs in a js_library rule to get the module-mapping to work. It would be much clearer and simplify the code if we started using a provider.

pauldraper commented 6 years ago

57

alexeagle commented 4 years ago

This is resolved in 1.5.0 which adds a LinkablePackageInfo provider for the purpose of exchanging the path and name of packages in your monorepo that can be npm link'ed into downstream compiles or programs.