bazelbuild / rules_closure

Closure rules for Bazel
https://developers.google.com/closure
Apache License 2.0
153 stars 114 forks source link

Stop given a depset as the first unnamed argument to the depset() con… #480

Open Yexo opened 4 years ago

Yexo commented 4 years ago

…structor.

This is in preparation for flipping the Bazel flag --incompatible_disable_depset_items to true, making it an error to use the "items" parameter. See https://github.com/bazelbuild/bazel/issues/9017 for details.

Yannic commented 4 years ago

Requiring a sequence instead of also accepting a depset for exports and internal_descriptors is a breaking change that I think we should try to avoid.

Looking at https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/457, it seems like rules_closure is compatible with --incompatible_disable_depset_items. Can you give more insights about why this is necessary?

Yexo commented 4 years ago

Requiring a sequence instead of also accepting a depset for exports and internal_descriptors is a breaking change that I think we should try to avoid.

It's only a change to _closure_js_library_impl, which is internal to this .bzl file and only used twice: First is from create_closure_js_library which uses neither exports nor internal_descriptiors, so this change is a no-op there. Second usage is from _closure_js_library which already passes in a list for both.

Looking at https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/457, it seems like rules_closure is compatible with --incompatible_disable_depset_items. Can you give more insights about why this is necessary?

I guess this is not strictly necesary. A recent warning I added to buildifier (https://github.com/bazelbuild/buildtools/commit/6827138c86919697c98133b84678b9176225ba8f) flagged this file as a violation (incorrectly, due to using mixed types) and while evaluating whether the warning was correct or not I figured I could simplify the code a bit.

gkdn commented 2 years ago

There hasn't been any activity in this PR for very long time. Please let me know if there are any intentions to pursue this change. Otherwise I will close it. Thanks.