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

Reorganize repo; TypeScript config improvements #3463

Merged
merged 8 commits into from
Dec 21, 2024
Merged

Reorganize repo; TypeScript config improvements #3463

merged 8 commits into from
Dec 21, 2024

Conversation

berekuk
Copy link
Collaborator

@berekuk berekuk commented Dec 20, 2024

1. packages/* is split into packages/, internal-packages/ and apps/

This is aligned with turborepo guidelines. (except that I decided that internal-packages is useful to keep separately, for clarity).

In the perfect world (or a bigger org) we'd have lint checks that packages don't depend on internal-packages. But not worth it for now, can check during reviews manually instead.

2. Rethinking tsconfigs

No cross-package TypeScript references

Previously, we used cross-package references:

"references": [
{ "path": "../squiggle-lang" },
{ "path": "../ui" },
{ "path": "../prettier-plugin" }
]

As mentioned in turborepo guides, this is not very useful and can be harmful:

  • turbo build would already take these dependencies into account, automatically, and cache the results
  • in rare cases, this could cause collisions in CI due to concurrency

${configDir}

  • TypeScript >=5.5 supports ${configDir} which allows us to set rootDir and srcDir in the base config (internal-packages/configs/tsconfig.base.json). This simplifies the config files.

3 -> 2 tsconfigs in most packages

Previously, in packages with tests, I used a pattern where we had three configs, e.g. in components:

  • tsconfig.json that referenced "build config" and "tests config"
  • tsconfig.tests.json which did type checks for all ts files in repo with noEmit enabled
  • and tsconfig.build.json which did builds, but only for src (since we don't want to build and package test files; also because building everything in the package resulted in the awkward dist/src/ paths)

Now I've managed to get the same result just two files:

Notes:

  • I've checked that the check config reuses tsbuildinfo caches from the build config, so there's no overhead from two files
  • one config still references another, to get rid of this performance overhead; that's fine, only cross-package references are bad
  • tsBuildInfoFile are configured explicitly; this is for aesthetical reasons, mostly, and I might change my mind in the future on how useful it is; explained here:
    /*
    * Recommended: define `tsBuildInfoFile` in each tsconfig separately to point to `./dist/...`.
    *
    * It's better to store tsbuildinfo files in `dist` than in package's root (easier to clean up).
    * We could define something like `"tsBuildInfoFile": "${configDir}/dist/main.tsbuildinfo"` here,
    * but then we'd risk collisions in packages that use multiple tsconfigs.
    */
  • packages without tests can get by with a single simple tsconfig.json, but if they have any *.ts configs in the package's root, and want to type-check them (we previously were not very consistent about this...) they'd end up with dist/src awkwardness
  • some packages are special, e.g. internal-packages/content uses a custom setup because it has to account for content-collections behavior

3. Cleanups

  • packages/cli (squiggle-cli-experimental) is removed, no one uses .squiggleU anymore
  • examples dir moved from root to packages/squiggle-lang/ (do we even need it? we have Squiggle Hub now...)
  • some updates to README and CONTRIBUTING

@berekuk berekuk requested a review from OAGr as a code owner December 20, 2024 18:09
Copy link

vercel bot commented Dec 20, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
quri-hub ❌ Failed (Inspect) Dec 20, 2024 6:54pm
squiggle-website ❌ Failed (Inspect) Dec 20, 2024 6:54pm
2 Skipped Deployments
Name Status Preview Updated (UTC)
quri-ui ⬜️ Ignored (Inspect) Visit Preview Dec 20, 2024 6:54pm
squiggle-components ⬜️ Ignored (Inspect) Visit Preview Dec 20, 2024 6:54pm

@berekuk
Copy link
Collaborator Author

berekuk commented Dec 20, 2024

Ugh, hub and website builds are failing, because packages/hub and packages/website are mentioned in Vercel settings.

I think we should merge this PR first, then I'll update the paths in terraform configs and redeploy manually.

@berekuk
Copy link
Collaborator Author

berekuk commented Dec 20, 2024

There's a weird css issue (probably related to versioned-components tailwind config) that I need to solve before this can be merged.

Fixed.

Copy link

changeset-bot bot commented Dec 20, 2024

⚠️ No Changeset found

Latest commit: bfeedb0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

@OAGr OAGr left a comment

Choose a reason for hiding this comment

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

If you think this is good, happy to go with it. Your explanation sounds reasonable.
I hope this helps with the Typescript complexity, I previously found that to be pretty annoying.

@berekuk berekuk merged commit 32efec9 into main Dec 21, 2024
4 of 6 checks passed
@berekuk berekuk deleted the organize-repo branch December 21, 2024 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants