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

feat(typescript): create a better ts_project worker #2416

Merged
merged 1 commit into from
Jan 30, 2021

Conversation

mrmeku
Copy link
Collaborator

@mrmeku mrmeku commented Jan 23, 2021

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

The last ts project worker I made was riddled with race bugs and overall didn't work that well.
Additionally it was not windows compatible.
Issue Number: N/A

What is the new behavior?

This variant of the ts_project worker uses the createWatchCompilerHost api directly rather than calling out to the typescript cli as a child process. It does not rely on program pausing and is thus windows compatible.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@mrmeku mrmeku requested review from mattem and soldair as code owners January 23, 2021 22:37
@google-cla google-cla bot added the cla: yes label Jan 23, 2021
@mrmeku mrmeku force-pushed the better_worker branch 10 times, most recently from 9f42e25 to 0233d61 Compare January 25, 2021 18:14
Copy link

@Wenqer Wenqer left a comment

Choose a reason for hiding this comment

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

Hey! Thanks for working on it! We recently started a migration to the Bazel, and this feature is the last significant blocker for us.
I left several comments/questions. I will hope they do not create much burden for you.

packages/typescript/internal/ts_project.bzl Outdated Show resolved Hide resolved
packages/typescript/internal/ts_project.bzl Outdated Show resolved Hide resolved
@lencioni
Copy link
Contributor

When I tried out the existing implementation of supports_workers, I (and others) have run into a problem where it does not find npm dependencies. Some more information is captured here: https://bazelbuild.slack.com/archives/CEZUUKQ6P/p1609246740191500

For me, when I add supports_workers = True, node_modules does not resolve during build time. And run with traceResolution: true shows that they just do not exist. However, when supports_workers is False, everything works fine.

According to what I debugged so far, the node_modules folder does not exist in my setup when I run ts_project with supports_workers = True.

With enabled sandbox and supports_workers = False, everything works fine.

@mrmeku Do you think your new implementation will handle this better? Can we add a test case that covers this scenario?

@mrmeku
Copy link
Collaborator Author

mrmeku commented Jan 26, 2021

When I tried out the existing implementation of supports_workers, I (and others) have run into a problem where it does not find npm dependencies. Some more information is captured here: https://bazelbuild.slack.com/archives/CEZUUKQ6P/p1609246740191500

For me, when I add supports_workers = True, node_modules does not resolve during build time. And run with traceResolution: true shows that they just do not exist. However, when supports_workers is False, everything works fine.
According to what I debugged so far, the node_modules folder does not exist in my setup when I run ts_project with supports_workers = True.
With enabled sandbox and supports_workers = False, everything works fine.

@mrmeku Do you think your new implementation will handle this better? Can we add a test case that covers this scenario?

Yes, I think that this type I got the logic right so that there shouldn't be any missing npm dependencies. The package does rely on typescript being a peer dependency so that's a requirement, but nothing else is required.

@mrmeku mrmeku force-pushed the better_worker branch 4 times, most recently from 7111c73 to c8be6a7 Compare January 26, 2021 03:03
Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

nice! excited to try this

examples/react_webpack/BUILD.bazel Outdated Show resolved Hide resolved
packages/typescript/internal/ts_project.bzl Outdated Show resolved Hide resolved
packages/typescript/internal/worker/worker_adapter.ts Outdated Show resolved Hide resolved
packages/typescript/internal/worker/worker_adapter.ts Outdated Show resolved Hide resolved
@mrmeku mrmeku force-pushed the better_worker branch 2 times, most recently from b9633de to b52bf9c Compare January 27, 2021 00:08
packages/typescript/internal/ts_project.bzl Show resolved Hide resolved
examples/react_webpack/tsconfig.json Outdated Show resolved Hide resolved
@mrmeku mrmeku force-pushed the better_worker branch 2 times, most recently from 7b54ee8 to 59b824e Compare January 27, 2021 20:17
@mrmeku mrmeku merged commit 99bfe5f into bazel-contrib:stable Jan 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants