Closed volkangurel closed 4 years ago
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).
:memo: Please visit https://cla.developers.google.com/ to sign.
Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!
and we'll verify it.
ℹ️ Googlers: Go here for more info.
@googlebot I signed it!
Hi, thanks Volkan!
My understanding of the associated issue is that this is an issue in TypeScript - they only understand module names that come from the automatic resolver.
For this change, I would have expected that the aspect we run to collect the module mappings should find all the external modules, so this workaround feels like it might be happening in the wrong layer. It seems like the BUILD file we generate for scoped typings packages should maybe expose the module_name and module_root so it happens to get picked up using the existing logic.
Do you have a small repro of the current issue?
(Also as a note, @gregmagolan and I have been working on a new implementation for ts_library that doesn't do any of this path manipulation, by making sure TS sees a node_modules directory that matches its expectations. But no estimate on when we might finish, depends on how we can get paid for it)
Hey Alex!
I have a small repro of the issue here: https://github.com/volkangurel/bazel-scoped-package-example. The first commit in the repo is the vanilla @bazel/create
and the second commit is a minimal set of changes to (fail to) build a typescript file that imports a scoped package.
To be honest I'm not very familiar with how BUILD files are generated for packages, but that sounds like a better place to implement this change for sure. Can you point me in the right direction on how I can go about developing something that will expose the module_name and module_root from these BUILD files? Is there an example of doing something similar somewhere?
Also looking forward to that new implementation you're thinking about!
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: https://github.com/bazelbuild/rules_nodejs/issues/1033
What is the new behavior?
The
create_tsconfig
function now has anexternal_module_roots
arg so that thets_library
rule can insert explicit paths intotsconfig.json
for scoped packages. The change forts_library
will be made as a separate PR once this change merges in (a potential change is like so https://github.com/volkangurel/rules_nodejs/commit/04420072a11a4cbbe69e3c0f3c6c25566025def8)Does this PR introduce a breaking change?
Other information
N/A