Page MenuHomePhabricator

[dagit] deps upgrade
ClosedPublic

Authored by alangenfeld on Jul 22 2019, 10:45 PM.

Details

Reviewers
bengotow
Group Reviewers
Restricted Project
Commits
R1:dda14898d254: [dagit] deps upgrade
Summary
  • yarn upgrade-interactive --latest to update all our deps to the latest version
  • move from react-scripts-ts to react-scripts
  • absorb D651 (tslint to eslint)
  • various changes to get stuff working

Note: this diff will not pass CI until it includes all the offline package changes which makes it entirely not reviewable

Test Plan

yarn lint
yarn ts
yarn jest
existing tests - explore and execute execute error monster, many events, and composition toy pipelines

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

alangenfeld created this revision.Jul 22 2019, 10:45 PM
alangenfeld added inline comments.Jul 22 2019, 10:49 PM
js_modules/dagit/tsconfig.json
1โ€“49

this

base diff without any offline package changes

alangenfeld edited the summary of this revision. (Show Details)Jul 22 2019, 10:59 PM
alangenfeld added reviewers: Restricted Project, bengotow.
alangenfeld edited the summary of this revision. (Show Details)
alangenfeld edited the summary of this revision. (Show Details)
alangenfeld edited the test plan for this revision. (Show Details)

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

alangenfeld updated this revision to Diff 3091.Jul 23 2019, 5:00 PM
alangenfeld edited the summary of this revision. (Show Details)
alangenfeld edited the test plan for this revision. (Show Details)

migrate of ts-jest to babel to maintain CRA support - fix jest

bengotow accepted this revision.Jul 23 2019, 10:13 PM

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...

This revision is now accepted and ready to land.Jul 23 2019, 10:13 PM
alangenfeld added inline comments.Jul 25 2019, 3:31 PM
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.

I have other react projects that use react-scripts and they don't add babel

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

alangenfeld updated this revision to Diff 3175.Jul 25 2019, 3:34 PM

fold in offline package cache changes

This revision was automatically updated to reflect the committed changes.