bazelbuild / rules_postcss

PostCSS rules for Bazel
Apache License 2.0
10 stars 13 forks source link

`postcss_multi_binary()` additional outputs #64

Open dgp1130 opened 3 years ago

dgp1130 commented 3 years ago

I noticed that postcss_multi_binary() does not seem to have an additional_outputs parameter like postcss_binary() does. This means the multi binary rule can't be used with a plugin that outputs additional files (such as postcss-modules, which is the motivating example).

Is this a deliberate omission due to some technical or philosophical reason, or is it just an oversight? I could probably put together a PR if we agree this is worth adding.

nex3 commented 3 years ago

Currently, all the outputs of postcss_multi_binary are homogeneous: they're just CSS files (plus maybe some source maps), so you can pass them to any rule that aggregates CSS. Once you add additional outputs, the resulting rule's outputs will have a bunch of different types of files that you can't explicitly refer to by name. It makes it harder to handle both the CSS, and the other outputs that you may want to send elsewhere.

That said, if it's worth the pain for you, I don't think it's unreasonable to add support for that feature.

dgp1130 commented 3 years ago

Thinking about this more, it's actually a bit tricky to leverage additional_outputs because it requires enumerating all the individual files being added. If a plugin exports a .json file for every .css file, then you need to individually list every .json file. This is impractical for use cases where you don't know all the files at loading time. For example:

# Aggregate a bunch of files, possibly in different packages.
filegroup(
    name = "css_files",
    srcs = ["..."],
)

postcss_multi_binary(
    name = "binary",
    srcs = [":css_files"],
    additional_outputs = ["???"], # Don't know what all the files in `:css_files` are.
    plugins = {
        "//some/plugin/emitting/a/json/file/for/each/css/input": "",
    },
)

Hypothetically, the same problem exists with additional_outputs in postcss_binary(), but that is probably less of an issue because it is only compiling one input file, so most likely the user knows exactly what that file is at loading time.

I wonder if a better approach might be to create a directory and then have plugins put excess files in that directory. Then it would be up to the user to extract the relevant files separately. Using the same example:

filegroup(
    name = "css_files",
    srcs = ["..."],
)

postcss_multi_binary(
    name = "binary",
    srcs = [":css_files"],
    # Creates a new `output_directory/` folder.
    # Plugins are responsible for writing additional outputs files here.
    additional_outputs_dir = "output_directory",
    plugins = {
        "//some/plugin/emitting/a/json/file/for/each/css/input": """[{
            // Configure the plugin to write to the additional outputs directory.
            outputDir: 'output_directory/',
        }]""",
    },
)

genrule(
    name = "consumer",
    # Depend on the additional outputs directory which contains a JSON file for each CSS input.
    srcs = ["output_directory"],
    outs = # ...
    cmd = """
        find $< -name "*.json" -type f -exec do-something-useful-with-json {} \;
    """
)

This way, postcss_multi_binary() creates a directory at output_directory/ and the plugin is configured to write its outputs to that directory. That directory can contain any number of additional outputs which don't need to be enumerated before the build. A consumer would just depend on that directory and use the files in whatever way it wanted. It's a bit inconvenient if the plugin is only outputting a single file, but users could write a fairly trivial genrule() to extract such a file.

postcss_multi_binary(
    name = "binary",
    additional_outputs_dir = "output_directory",
    # ...
)

genrule(
    name = "consumer",
    srcs = ["output_directory"],
    outs = ["special_file.txt"],
    cmd = "cp $(location output_directory)/some/path/to/a/special/file.txt $(location special_file.txt)",
)

I think configuring plugins is the most awkward part of this since the plugin must support writing to a specific location. It is also awkward because output_directory/ probably exists at bazel-out/${configuration}/bin/path/to/pkg/output_directory/, and that path will change depending on the configuration of the postcss_multi_binary(). So either postcss_multi_binary() needs to set up a stable symlink to this location (such as $PWD/output_directory/), or it needs to provide a replacement/variable for configurations to use. Maybe the plugin could define const additionalOutputsDir = // ... in scope before loading plugins. Then the plugin configuration could just use additionalOutputsDir to specify where to write the files. Otherwise it would need to use a text replacement like %{additional_outputs_dir} and substitute the real directory in the plugin configuration string.

To your point about postcss_multi_binary() outputs being pure CSS / sourcemap files and an additional_outputs parameter diluting them, I think it could make sense to not return additional outputs in DefaultInfo. That way depending on a binary directly would still give you only CSS / sourcemap files, and you would have to directly depend on the additional output directory to get any of those files. I think that's a reasonable ask for consumers and keeps the regular output pure.

Do you have any thoughts about an additional_outputs_dir approach? Is that too complex of an API or is it worth the effort to support non-file references as inputs?

dgp1130 commented 3 years ago

Related: https://github.com/bazelbuild/rules_postcss/issues/63#issuecomment-927230185.