| bengotow |
| Restricted Project |
Note: this diff will not pass CI until it includes all the offline package changes which makes it entirely not reviewable
yarn lint
yarn ts
yarn jest
existing tests - explore and execute execute error monster, many events, and composition toy pipelines
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
| js_modules/dagit/tsconfig.json | ||
|---|---|---|
| 1–49 | this | |
once i get a sign off on these changes I will start updating the diff to get it passing CI
| js_modules/dagit/.eslintrc.js | ||
|---|---|---|
| 20–30 | a lot of these can get turned back on in subsequent diffs | |
This looks great! Thanks for slogging through this. Made a few comments inline but nothing blocking.
| js_modules/dagit/package.json | ||
|---|---|---|
| 71 | Still need to pull this down and try it but it looks like we're just using babel for jest now? (It looks like I have other react projects that use react-scripts and they don't add babel, just typescript, so I don't think it's necessary to declare it here for the build process). If that's the case, we might consider swapping out babel-jest for ts-jest (https://github.com/kulshekhar/ts-jest) so that the tests use the same pre-processor and there aren't scenarios where some random ES2018 feature will build in tsc but not in babel? I /think/ that ts-jest can be a drop in replacement in the transform below. | |
| js_modules/dagit/src/graph/MappingLine.tsx | ||
| 33 | Ahh this is great, this formatting has been driving me crazy. | |
| js_modules/dagit/src/graph/SolidLinks.tsx | ||
| 34 | This one is a small behavioral change (I think that previously this allowed you to hover the mouse over a connection between solids and see that string). Probably OK though | |
| js_modules/dagit/tsconfig.json | ||
| 28 | Man the TS folks are really going to town with options these days - it's gone from ~8 to 50 in like a year 😬. These all look good to me. It'd be kind of nice to move to strict:true eventually, since lines [16-19] are enabling all of strict mode except 1 or 2 features, but that might involve morer work... | |
| js_modules/dagit/.eslintrc.js | ||
|---|---|---|
| 20–30 | [2] | |
| js_modules/dagit/package.json | ||
| 71 | the issue is that the current react-scripts typescript support is babel based and conflicts with ts-jest https://github.com/kulshekhar/ts-jest/issues/1118 since react-scripts-ts is now deprecated, it seems more valuable to move forward on the react-scripts train even though we lose ts-jest which i think you could argue is slightly better since it runs type checks as part of the tests suite. If we want to eject from react-scripts we can revisit alot of this.
If we were using react-scripts to drive the tests i think we would be fine - but our current set up invokes jest directly and we dont conform to the react-scripts way of laying out our tests [1] so moving to react-scripts test would have been a bigger change than setting up babel here | |
| 122 | [1] | |
| js_modules/dagit/src/graph/SolidLinks.tsx | ||
| 34 | StyledLink doesnt take children anymore - maybe there is a prop where the hover text can be supplied | |
| js_modules/dagit/tsconfig.json | ||
| 28 | ya I think cleaning up the lint rules we wish were on [2] and getting strict true on are not that out of reach - just too much for this diff | |