refactor: do not add JsInfo.types or js_library(types) to JsInfo.sources #2080
Conversation
|
jbedard
changed the title
jbedard
force-pushed
the
types-not-sources
branch
from
b6a6cca to
87fbd6c
Compare
| # Collect transitive sources, types and npm providers from srcs+types+deps | ||
| for target in srcs_types_deps: | ||
| # Collect transitive sources, types and npm providers from js_library(srcs+deps) | ||
| for target in srcs_deps: | ||
| if JsInfo in target: | ||
| jsinfo = target[JsInfo] | ||
| transitive_sources.append(jsinfo.transitive_sources) | ||
| transitive_types.append(jsinfo.transitive_types) | ||
| npm_sources.append(jsinfo.npm_sources) |
There was a problem hiding this comment.
Targets in types may have npm deps they depend on so I think it is correct to propagate the npm_package_store infos as well.
Technically, this PR is a breaking change as we're changing the contract for the types attribute:
"""Same as `srcs` except all files are also provided as "types" available to downstream rules for type checking.
For example, a js_library with only `.js` files that are intended to be imported as `.js` files by downstream type checking
rules such as `ts_project` would list those files in `types`:
js_library(
name = "js_lib",
types = ["index.js"],
)
"""
With this change it is no longer "Same as srcs" but instead deps added to types only propogate their types and not their sources. That does seem like a better pattern for that attribute... essentially if you wanted something that wouldn't be normally be a type to be included in types then you would put it in types. If you also wanted it in sources then you would put it in both types and sources. With this change you can pass a dep with both sources and types and intentionally strip out the sources.
However, the question is how breaking is this for users? My guess is that very few ppl are using the types attribute and this could be marked as a "fix" under the reasoning that the past behaviour was not correct.
There was a problem hiding this comment.
Semver police might overlook this one :)
No "types" should ever make it into runfiles or
JsInfo.[transitive_]sourcesas it might trigger type-producing actions such as tsc.I'm uncertain if packages in
js_library(types)should be passed forward toJsInfo.{npm_sources,npm_package_store_infos}though.Changes are visible to end-users: yes
Do not propagate
js_library(types)orJsInfo.[transitive_]typestoJsInfo.sources.Test plan