Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test case for ts/js interop. #712

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aptenodytes-forsteri
Copy link


Changes are visible to end-users: yes/no

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

Test plan

  • Covered by existing test cases
  • New test cases added
  • Manual testing; please provide instructions so we can reproduce:

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@jbedard jbedard self-requested a review October 31, 2024 20:53
Copy link

aspect-workflows bot commented Oct 31, 2024

Test

⚠️ GitHub Actions build #968 failed.


Buildifier

Buildifier lint check has failed

./examples/ts_js_interop/BUILD.bazel:2: out-of-order-load: Load statement is out of its lexicographical order. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#out-of-order-load)

💡 To reproduce the buildifier failures, run

bazel run //:buildifier.check

Some lint failures can be fixed automatically by running the following while other require manual fixes

bazel run //:buildifier

📝 For more information on how to resolve or suppress specific lint failures, see the Buildifier documentation


Format

Formatting check has failed

💡 Some formatting failures can be fixed automatically by running the command below, while others may require manual fixes

bazel run //:format

)

filegroup(
name = "transpiled_a_b",
Copy link
Member

Choose a reason for hiding this comment

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

I think these filegroup targets are never used.

Can we replace those with build_test targets so bazel test //... triggers them and they showup in the test results?

Choose a reason for hiding this comment

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

I'm not familiar with build_test targets, but probably? Have changes gone in that allow these filegroups to build successfully now?

Copy link
Member

Choose a reason for hiding this comment

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

Search for build_test in other tests for examples and try switching these filegroup to test targets.

Choose a reason for hiding this comment

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

build_test is a trivial test that always passes.

I see, makes sense. It's a trivial test that will always pass as long as it builds.

Copy link
Member

Choose a reason for hiding this comment

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

Correct, I've almost exclusively used it for testing rules just like this test you've written.

@jbedard
Copy link
Member

jbedard commented Oct 31, 2024

Sorry I forgot about this. Just 1 minor comment and a buildifier error now and then I think we can merge this.

@aptenodytes-forsteri
Copy link
Author

Sorry I forgot about this. Just 1 minor comment and a buildifier error now and then I think we can merge this.

Should we link this PR to the commits that made these test cases pass?

@jbedard
Copy link
Member

jbedard commented Nov 1, 2024

Should we link this PR to the commits that made these test cases pass?

You can put links to the commits in the PR description 👍

@aptenodytes-forsteri
Copy link
Author

Should we link this PR to the commits that made these test cases pass?

You can put links to the commits in the PR description 👍

Sorry, I don't know what commits fixed this. I figure you know because you changed something to make these tests pass?

@jbedard
Copy link
Member

jbedard commented Nov 1, 2024

Oh I see, sorry I don't recall either.

If you want you could bisect to determine which one did it, I assume that wouldn't take long (and your tests are new so you can just leave them not checked in while bisect-ing).

@jbedard
Copy link
Member

jbedard commented Nov 1, 2024

@aptenodytes-forsteri
Copy link
Author

aptenodytes-forsteri commented Nov 1, 2024

Have we confirmed the tests do indeed pass? I haven't returned to this since I made commits a while back.

Actually it was probably this change: dd832db#diff-fd5b55e792f846b8312680267efd413bf73bfe655630485e1514db77eb7d187cL329-R370 ?

This timeline doesn't make sense to me. I have this commit in my fork of the repo where these tests fail. https://github.com/aptenodytes-forsteri/rules_ts/commits/ts-js-interop/

I was expecting something like: "The root cause of these test failures is code in file X at line Y. I have resolved this issue by making commit Z. The tests now pass and we can merge in this PR."

I'm not as familiar with this codebase or what the intended behavior is so I was hoping a maintainer could help root cause this issue or explain how I'm using it wrong.

@aptenodytes-forsteri
Copy link
Author

Basically, my ask since I produced the reproduction repo, opened the bug report, and opened this PR is for a maintainer of this code to pull down these reproduction cases and root cause why I can't seem to use ts and js seamlessly inside a ts_project. I currently have a workaround where I sort of "gut" ts_project and use parts of it in a separate macro when working with ts and js. The hope is that ts_project "just works" and I can get rid of my workaround macro.

@aptenodytes-forsteri
Copy link
Author

It seems like this run correctly reproduced both errors I see:

https://github.com/aspect-build/rules_ts/actions/runs/11327617927/job/32359980802?pr=712

So presumably the issue still persists and needs a root cause explanation and fix.

@jbedard
Copy link
Member

jbedard commented Nov 1, 2024

If you've reproduced a bug then I can take a look, and a good test case like this is the best way to demonstrate an issue 👍

I'll try to look more tomorrow.

@jbedard
Copy link
Member

jbedard commented Nov 2, 2024

I'm wondering how much of this is just a limitation of bazel and rules_ts...

With allowJs we still need to copy the files into bazel-bin so they can be included for type-checking. That means we can't transpile them to the same location and would need to use outDir so the input != output.

],
allow_js = True,
declaration = True,
emit_declaration_only = True,
Copy link
Member

Choose a reason for hiding this comment

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

When using emitDeclarationOnly the transpiler won't run at all, tsc or a custom transpiler like swc, so b.js should not be expected at all

Choose a reason for hiding this comment

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

Right, I figured as much. So this test case can be deleted and clearly does not solve my problem. I only included it to point out that this is one way to avoid the conflict error, but I need b.js to be produced in my environment.

@aptenodytes-forsteri
Copy link
Author

I'm wondering how much of this is just a limitation of bazel and rules_ts...

With allowJs we still need to copy the files into bazel-bin so they can be included for type-checking. That means we can't transpile them to the same location and would need to use outDir so the input != output.

Right this is sort of what I'm getting at. I know it's a tricky case, but I want to transpile the js files and produce the .d.ts files and include them for typechecking.

My workaround involves a macro that filters out js sources into a separate swc and tsc macro which eventually get passed to a js library.

Then, if there are ts sources, too, I create a ts_project that depends on the js_library

@aptenodytes-forsteri
Copy link
Author

With allowJs we still need to copy the files into bazel-bin so they can be included for type-checking

Could there be some conditional logic where, if the sources are js, you only copy the transpiled files to bazel-bin, not the raw source?

@jbedard
Copy link
Member

jbedard commented Nov 10, 2024

That makes the transpiler case diverge even more though, which I'd like to avoid :/

@aptenodytes-forsteri
Copy link
Author

That makes the transpiler case diverge even more though, which I'd like to avoid :/

Another option: check for and explicitly disallow this case (which I guess is custom transpiler with a mix of ts and js sources)? Throw an error explaining that when using a custom transpiler you can't do allowJs and can't pass in js files to the ts_project? And perhaps provide an example of a workaround of a wrapper macro to filter out the js files and pass them to separate swc and tsc rules?

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.

3 participants