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

Set up multiple typescript projects #9

Merged
merged 17 commits into from
Mar 20, 2023

Conversation

psrpinto
Copy link
Member

@psrpinto psrpinto commented Mar 14, 2023

(Not opened yet upstream as it depends on element-hq#1056 so must wait for that one to be merged first)

Requires #11


In the current typescript setup, attempting to use the WebWorker typescript library will not work, as that library is not part of typescript's default libraries. Attempting to require the library through triple-slash directives will result in an error:

// sync-worker.ts

/// <reference lib="WebWorker" />
Definitions of the following identifiers conflict with those in another file:
ImportExportKind, TableKind, ValueType ...

This happens because the WebWorker and DOM (which is part of the default libraries) libs define some of the same types. I consulted with a colleague who has experience with Web Workers in typescript and they recommended using typescript's project references feature to address this issue.

This PR "splits" the codebase into three typescript projects, which share common configuration, but can also have their own configuration to, e.g. import libraries. These projects are:

  1. app: Contains all code except workers. Requires the DOM and DOM.iterable libs
  2. workers: Contains workers only. Requires the WebWorkers lib. References the app project.
  3. sdk: Contains all code except workers. Requires the DOM and DOM.iterable libs. Only emits declarations.

The top-level tsconfig.json only contains references to each project.

Caveats

  1. When using project references, the tsc CLI command requires the --build flag. Not passing the --build flag will result in success, but no code has been checked/compiled.
  2. Project references require code to be emitted, even if you just want to lint. Compiled files are placed in a .typescript directory.

@psrpinto psrpinto self-assigned this Mar 14, 2023
@psrpinto psrpinto marked this pull request as ready for review March 14, 2023 16:30
@psrpinto psrpinto changed the base branch from sync-worker-kickoff to master March 20, 2023 13:55
@psrpinto psrpinto requested a review from ashfame March 20, 2023 13:55
@psrpinto psrpinto mentioned this pull request Mar 20, 2023
4 tasks
@psrpinto psrpinto marked this pull request as draft March 20, 2023 16:39
@psrpinto psrpinto changed the base branch from master to integration-sync-worker March 20, 2023 16:50
@psrpinto psrpinto marked this pull request as ready for review March 20, 2023 16:50
Copy link
Member

@ashfame ashfame left a comment

Choose a reason for hiding this comment

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

No comments from my side as I don't know TypeScript. Sail away ⛵

@psrpinto psrpinto force-pushed the workers-typescript branch from 0aebf99 to ea0c336 Compare March 20, 2023 16:51
@psrpinto
Copy link
Member Author

I rebased this on top of integration-sync-worker, thus the force-push, didn't change any code.

@psrpinto psrpinto merged commit 37355d9 into integration-sync-worker Mar 20, 2023
@ashfame
Copy link
Member

ashfame commented Mar 20, 2023

@psrpinto You shouldn't need to rebase your "sync in worker" PRs on integration-sync-worker for creating PRs unless you had a specific reason for doing that.

@psrpinto
Copy link
Member Author

Indeed there was no need to do so, and I won't do it with future PRs. It's just that this branch was created before the integration-sync-worker existed and I wanted to make sure it was based on that branch. In practice nothing changed, except for the commit hashes.

@psrpinto psrpinto deleted the workers-typescript branch March 21, 2023 15:58
@psrpinto
Copy link
Member Author

psrpinto commented Mar 21, 2023

This PR was reverted (by force-push) on integration-sync-worker, since it turns out this approach does not fix the issues with workers in typescript we were trying to address.

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