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

Infer module_name and module_root from providers. #278

Closed
hsyed opened this issue Aug 13, 2018 · 9 comments
Closed

Infer module_name and module_root from providers. #278

hsyed opened this issue Aug 13, 2018 · 9 comments

Comments

@hsyed
Copy link

hsyed commented Aug 13, 2018

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
Copy link
Collaborator

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
Copy link
Author

hsyed commented Aug 14, 2018

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
Copy link
Collaborator

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
Copy link
Author

hsyed commented Aug 15, 2018

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
Copy link
Author

hsyed commented Aug 15, 2018

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
Copy link
Collaborator

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
Copy link
Contributor

@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
Copy link

#57

@alexeagle
Copy link
Collaborator

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.

alexeagle added a commit to alexeagle/rules_nodejs that referenced this issue Oct 17, 2020
alexeagle added a commit to alexeagle/rules_nodejs that referenced this issue Oct 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants