Conversation
🦋 Changeset detectedLatest commit: 58e07df The changes in this PR will be included in the next version bump. This PR includes changesets to release 24 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
👋 Hi @lishaduck, thanks for the pull request! A scan flagged a concern with it. Could you please take a look? [pr-task-completion] This PR's body is missing
Repositories often provide a set of tasks that pull request authors are expected to complete. Those tasks should be marked as completed with a
|
aec299e to
839cb01
Compare
839cb01 to
47557ae
Compare
Well, it's fixed now (hehe), but #1151 is really really broken instead 😬 |
<!-- 👋 Hi, thanks for sending a PR to flint! ❤️🔥 Please fill out all fields below and make sure each item is true and [x] checked. Otherwise we may not be able to review your PR. --> ## PR Checklist - [x] Addresses an existing open issue: fixes #1139 - [x] That issue was marked as [`status: accepting prs`](https://github.com/JoshuaKGoldberg/flint/issues?q=is%3Aopen+is%3Aissue+label%3A%22status%3A+accepting+prs%22) - [x] Steps in [CONTRIBUTING.md](https://github.com/JoshuaKGoldberg/flint/blob/main/.github/CONTRIBUTING.md) were taken ## Overview <!-- Description of what is changed and how the code change does that. --> Extracted from #1129 🔥
Internal lint rule could help with that! Especially with an autofixer |
|
Should I make a changeset? This isn't forward-facing, but we should probably ensure we republish all of the packages. |
|
Yeah, just to be safe let's do that. |
| "compilerOptions": { | ||
| "outDir": "lib", | ||
| "rootDir": "src" | ||
| "tsBuildInfoFile": "node_modules/.cache/tsbuild/info.json", |
There was a problem hiding this comment.
[Question] Are these necessary? Why not go with the default?
There was a problem hiding this comment.
Yes, we require setting them to disambiguate between , .test, & .src. Putting them in node_modules/.cache/tsbuild was a matter of preference (to prevent #1129 (comment) and to collocate with the tsbuild output). We could totally put them in as tsconfig.tsbuildinfo, tsconfig.src.tsbuildinfo, tsconfig.test.tsbuildinfo, tsconfig.bin.tsbuildinfo, but that gets noisy quickly.
There was a problem hiding this comment.
Hmm. I was a little uneasy with the @flint.fyi/source in package.json, and now this is a second unusual (to me?) configuration point I haven't seen used elsewhere. What is it about our setups that's forcing these unusual config options uses?
There was a problem hiding this comment.
I was a little uneasy with the
@flint.fyi/sourceinpackage.json
That's fair, but I'd like to note that offical TS team recommends it as the way to use type-stripping. It's not so much unusual, it's that most projects of our scale haven't migrated to erasableSyntaxOnly, so it's only smaller projects like tinyglobby that use it (irrc tinyglobby uses it, don't quote me, I didn't double-check).
(to me?) configuration point I haven't seen used elsewhere
Really? tbh I'd never seen a project (before Flint) that didn't customize tsbuildinfo when using incremental.
What is it about our setups that's forcing these unusual config options uses?
- Being relatively non-trivial
- Being a monorepo
- Adopting newer features most non-trivial monorepos haven't migrated to yet.
Again, I don't think it's so much that options are unusual, it's that the projects you use and the projects I use are generally of different scales.
I was curious, so I ran a quick sourcegraph search of tsBuildInfoFile, which returns >10k results for tsconfig.json, so, again, I don't think it's very unusual to set it, just that it might be unusual in your circles.
EDIT: typescript-eslint uses it :) (not much, surprisingly. I'm curious how they do it now...) https://github.com/search?q=repo%3Atypescript-eslint%2Ftypescript-eslint%20tsBuildInfoFile&type=code
There was a problem hiding this comment.
EDIT: typescript-eslint uses it :) (not much, surprisingly. I'm curious how they do it now...) https://github.com/search?q=repo%3Atypescript-eslint%2Ftypescript-eslint%20tsBuildInfoFile&type=code
I took a closer look to see how typescript-eslint was using it, given I assume you'd be more comfortable with that approach. It looks like in typescript-eslint, y'all are doing the same thing I'm doing, but you just do it in the more centralized (and thus implicit) manner.
I don't totally love the approach y'all took to centralize it (I don't love how crowded it makes the root, for one), but now that I see how y'all did it, I think I could adapt it pretty nicely here as well.
I assume you'd like that, I'd like that too, I don't know if we want to make it block getting this in. On one hand, auvred said he'd prefer this to land before #1179, but at the same time, a followup would be equally conflicty (though less so than #1201).
I say that because I'm going to try to resist working on Flint tomorrow (gotta finish polishing my WashU app & get back to the MIT app), but we'll see. I don't see myself resisting the lure of code for more than a day though, so 🫡
G'night! Happy new years! (3 more minutes for me 🎉)
There was a problem hiding this comment.
I assume you'd like that
Heh, I wasn't on point for setting up tseslint's tooling. That was JamesHenry. I just ramp up to review the PRs once in a while and maybe try to tweak things when they break.
This whole area isn't something I'm very ramped up on. And IMO none of this is blocking - we can always change it later!
Proposal: how about we...
- Wait today since you're working on your future (whoo! 🎓)
- See if @auvred has preferences
- Go with whatever you two prefer tomorrow?
I'm personally happy as long as it all works. From my perspective, it's a win-win:
- If we do it a way I'm accustomed to: I'm comfortable
- If we do it some new way: I learn a new thing and get to try it out for reference later
There was a problem hiding this comment.
I'm personally happy as long as it all works.
+1! This is a tooling setup, we can always change it at any moment if we find something we don't like. Let's merge it?
There was a problem hiding this comment.
!
@lishaduck whenever you feel comfortable, all you!
There was a problem hiding this comment.
Sounds good to me! ![]()
(I am aware that there are conflicts. Feel free to fix them yourself if it's blocking something, or else I'll get it to tomorrow)
No need for this anymore 🧹
|
I tried the tsdown 0.19 beta, but the exports generation still isn't quite where we want it. Put up rolldown/tsdown#685 to fix that. I think we leave it for a followup like #1129 (comment). EDIT: Filed #1251 |
|
Self merging once CI passed given everyone approves and this is particularly conflict-prone. |
<!-- 👋 Hi, thanks for sending a PR to flint! ❤️🔥 Please fill out all fields below and make sure each item is true and [x] checked. Otherwise we may not be able to review your PR. --> ## PR Checklist - [x] Addresses an existing open issue: fixes #1152 - [x] That issue was marked as [`status: accepting prs`](https://github.com/JoshuaKGoldberg/flint/issues?q=is%3Aopen+is%3Aissue+label%3A%22status%3A+accepting+prs%22) - [x] Steps in [CONTRIBUTING.md](https://github.com/JoshuaKGoldberg/flint/blob/main/.github/CONTRIBUTING.md) were taken ## Overview <!-- Description of what is changed and how the code change does that. --> Relands #1174. Marking as draft, despite being ready for review, to prevent accidental remerges before #1129. 🔥 --------- Co-authored-by: Josh Goldberg <git@joshuakgoldberg.com>
<!-- 👋 Hi, thanks for sending a PR to flint! ❤️🔥 Please fill out all fields below and make sure each item is true and [x] checked. Otherwise we may not be able to review your PR. --> ## PR Checklist - [x] Addresses an existing open issue: fixes #1248 - [x] That issue was marked as [`status: accepting prs`](https://github.com/flint-fyi/flint/issues?q=is%3Aopen+is%3Aissue+label%3A%22status%3A+accepting+prs%22) - [x] Steps in [CONTRIBUTING.md](https://github.com/flint-fyi/flint/blob/main/.github/CONTRIBUTING.md) were taken ## Overview <!-- Description of what is changed and how the code change does that. --> Fixes broken merge of #1129 & #1201
PR Checklist
status: accepting prsOverview
Should still probably get split up...