refactor: update to rules_js 1.0.0-rc.4 #113
Conversation
gregmagolan
force-pushed
the
rules_js_rc4
branch
4 times, most recently
from
6faf6bc to
c44bc38
Compare
gregmagolan
changed the title
gregmagolan
force-pushed
the
rules_js_rc4
branch
5 times, most recently
from
ee30c00 to
ec5245c
Compare
| @@ -12,7 +12,7 @@ ValidOptionsInfo = provider( | |||
|
|
|||
| # Targets in deps must provide one or the other of these | |||
| DEPS_PROVIDERS = [ | |||
| [DeclarationInfo], | |||
There was a problem hiding this comment.
https://en.wikipedia.org/wiki/Robustness_principle suggests that we should consider keeping DeclarationInfo here as an option. I think it's pretty trivial in the implementation to check it if JsInfo isn't present on a dep
There was a problem hiding this comment.
One problem with that is that we'd be advertising interop when we wouldn't be picking up the transitive npm dependencies from any ts_projects the produce DeclarationInfo
There was a problem hiding this comment.
yes that will be confusing for users that some deps have different behavior. I'm okay with this like it is
ts/private/ts_project.bzl
Outdated
| else: | ||
| # We must avoid tsc writing any JS files in this case, as tsc was only run for typings, and some other | ||
| # action will try to write the JS files. We must avoid collisions where two actions write the same file. | ||
| arguments.add("--emitDeclarationOnly") | ||
|
|
||
| # We don't produce any DefaultInfo outputs in this case, because we avoid running the tsc action | ||
| # unless the DeclarationInfo is requested. | ||
| default_outputs_depset = depset([]) | ||
| # unless the declarations are requested. |
There was a problem hiding this comment.
did you verify if this continues to work? If some downstream action asks for the JsInfo but doesn't need declarations, are we now going to trigger typechecks where we didn't previously?
(that may be a critical reason to keep the DeclarationInfo separate from JsInfo)
There was a problem hiding this comment.
I'll double check but yes it should work. A declaration file is only created if it ends up as an input to an action. Having File objects passed around in declarations and transitive_declarations in JsInfo doesn't trigger the actions unless they are passed as inputs to actions that need to run or as runfiles to binary targets that are run.
There was a problem hiding this comment.
I verified this locally and pretty sure it does work correctly, since include_declarations is False by default
gregmagolan
force-pushed
the
rules_js_rc4
branch
4 times, most recently
from
9183fa1 to
21faff5
Compare
gregmagolan
force-pushed
the
rules_js_rc4
branch
5 times, most recently
from
142bc92 to
5dfa5f4
Compare
gregmagolan
force-pushed
the
rules_js_rc4
branch
from
5dfa5f4 to
5c092de
Compare
gregmagolan
force-pushed
the
rules_js_rc4
branch
2 times, most recently
from
ef164f9 to
86668b6
Compare
gregmagolan
force-pushed
the
rules_js_rc4
branch
from
86668b6 to
491ff17
Compare
rc4 is not out yet but this update to what the derivate rules API will look like when it is released