Page MenuHomePhabricator

Docs to Typesript
AbandonedPublic

Authored by max on Mar 21 2020, 11:16 PM.

Details

Summary

Converted Docs to Typescript

Test Plan

Run a master branch (JS-Based) against this branch and check against Typescript (Already did locally)

Diff Detail

Repository
R1 dagster
Branch
docs-typescript
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

sashank requested changes to this revision.EditedMar 24 2020, 4:28 AM

This is great work, thanks! This was definitely a lot to churn through and I totally understand the any usage - it's good to get this working in typescript first and we can iteratively improve the typing for those in future diffs :)

Just a few things:

  • I do see a bunch of TODOs - it would be great to get them into an external list so we can track them as we address them in future diffs. Feel free to use the projects github issues and paste the issue link next to the TODO.
  • Most of the ts-ignores have comments (thank you!), but there's a few where it's not clear why they're there and it would be helpful to have a brief comment for future reference.
  • Noted a few places where it would be nice to have better types in this pass
  • If possible, we should try to diagnose why the eslint settings from this project is affecting dagit.

Overall, awesome work! Let me know if you have any questions about any comments.

docs/gatsby/.eslintrc.js
9

I'm not sure if this is the reason that arc lint is complaining about jsx-a11y in dagit, but I don't see any other changes that would cause that.

This is in reference to [1]

docs/gatsby/package.json
60–66

🙌

docs/gatsby/src/gatsby-plugin-theme-ui/index.ts
11–12

Would it be possible to add a comment here about why we are ts-ignoring?

docs/gatsby/src/systems/Core/CodeSection/index.tsx
9–11

It would be really nice to get actual types here if possible, we use this node heavily.

docs/gatsby/src/systems/Core/CodeSection/styles.ts
4

This seems a bit rough to do a null check and type cast every-time we want to use a color - would it be possible to clean this type up to prevent that?

docs/gatsby/src/systems/Core/Details/index.tsx
6–8

Just so that I understand IntrinsicElements - does this mean that children can only be a <dl> component?

Pretty neat - learned something new :)

docs/gatsby/src/systems/Core/Layout/styles.ts
3–4

Is this any related to the todo in docs/gatsby/src/systems/Core/Layout/machines/responsive.ts

docs/gatsby/src/systems/Core/Pre/index.tsx
32–33

Could we add a comment for this ts-ignore

docs/gatsby/src/systems/Header/index.tsx
53–54

Comment

docs/gatsby/src/systems/Search/components/Results/index.tsx
10–12
docs/gatsby/src/systems/Search/index.tsx
57–58

Comment

docs/gatsby/src/systems/Version/components/VersionSelector/styles.ts
1–2

This is great

docs/gatsby/src/templates/SphinxPage/index.tsx
15

This is another one where we would would get a lot of value by not having any

js_modules/dagit/package.json
87

[1] It would be great to figure out why the estlint rc is affecting dagit, so that we don't have problems with the two projects interfering with each other in the future. The goal here would be to not have any lint errors if we remove this package.

This revision now requires changes to proceed.Mar 24 2020, 4:28 AM
kevinrodriguez marked 8 inline comments as done.

code review changes (docs to typescript)

Adding changes to this diff

Adding changes to the diff

Adding changes from code-review

sashank accepted this revision.Mar 25 2020, 5:27 PM

Looks great. Added some followups for unaddressed feedback - feel free to address those here or in a future diff, and land this whenever you feel ready :)

docs/gatsby/src/systems/Core/CodeSection/index.tsx
9–11

nice!

docs/gatsby/src/systems/Core/CodeSection/styles.ts
4

^ Following up on this

docs/gatsby/src/systems/Core/Layout/styles.ts
3–4

^ Following up on this

docs/gatsby/src/systems/Search/components/Results/index.tsx
10–12

^ Also following up on this

This revision is now accepted and ready to land.Mar 25 2020, 5:27 PM
max commandeered this revision.Apr 21 2020, 7:03 PM
max abandoned this revision.
max edited reviewers, added: kevinrodriguez; removed: max.