Skip to content

Conversation

jbedard
Copy link
Member

@jbedard jbedard commented Apr 29, 2025

#746 changed this logic to align with outputted .js files which I think makes sense. However js also has the allowJs logic while dts files do not.


Changes are visible to end-users: yes

  • Searched for relevant documentation and updated as needed: yes
  • Breaking change (forces users to change their own code or config): no
  • Suggested release notes appear below: yes/no

Test plan

  • Covered by existing test cases
  • New test cases added

Copy link

aspect-workflows bot commented Apr 29, 2025

Test

All tests were cache hits

176 tests (100.0%) were fully cached saving 12s.


Buildifier      Format

output_types.append(s)
else:
# Add DTS inputs that are not transpiled by tsc yet should be copied to the out_dir
# See https://github.com/microsoft/TypeScript/issues/39231 for tsc request to add this feature
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If tsc doesn't support this... should rules_ts? Overall I feel like this workflow makes sense so it's odd tsc seems to not support it. Does the use of ts within bazel make this issue more common which would justify adding this feature despite tsc not supporting it? (imo the answer is yes)

As I understand it the scenario would be:

src/
   a.ts
   types.d.ts

Then ts_project(root_dir="src", out_dir="dist").

If the types within types.d.ts are public then the ts_project output really does need them (because a.d.ts probably imports types.d.ts).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally the principle here is that we behave the same as tsc as much as possible.
If someone needs to do this, can't they do it in userspace?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think users would have to use copy_to_bin or ts_project(assets) to copy the file to the bindir while moving it from rootDir to outDir. But then it won't be in JsInfo.types so it probably won't work when depended on.

That just seems odd. I think this only works outside bazel because the file will still be referenced in src all the time?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a lot of "probably" and asking questions, are you asking me to answer those questions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking for feedback and opinions because I'm uncertain about this change. I don't like adding functionality, but I don't think such a standard use case should require an ugly workaround in userspace.

This also partially (but incorrectly) worked in the past before #746, and it still (correctly) works if out_dir == root_dir.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So tsc explicitly doesn't not copy .d.ts files, unlike .js when allowJs. This was intentional from the beginning: microsoft/TypeScript#5112 (comment)

Outside bazel this normally isn't an issue because a) no sandboxing (so you can access the .d.ts even if it isn't outputted to outDir) and b) pnpm symlinks directly into the source for references between projects so these .d.ts are available there as well.

In bazel the non-copied .d.ts are not accessible at all: they are not in the sandbox, they are not in npm packages. Yet they would be copied if in js_library(srcs) which makes this even more odd...?

@jbedard jbedard marked this pull request as ready for review April 29, 2025 02:52
@thesayyn thesayyn removed their request for review July 9, 2025 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants