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

@damassi => Upgrades typescript and enables type emitting #715

Merged
merged 9 commits into from Jun 4, 2018

Conversation

l2succes
Copy link
Contributor

@l2succes l2succes commented May 31, 2018

screen shot 2018-05-31 at 5 31 33 pm
This allows us to emit type definitions alongside compiled js files in our reaction builds so consumers that also use typescript (e.g. 'force') have access to them.

TODO

  • swap build from babel 7 back to tsc

@l2succes l2succes requested a review from damassi May 31, 2018 21:38
tsconfig.json Outdated
"Assets/*": [
"./Assets/*"
]
"Assets/*": ["./Assets/*"]
Copy link
Member

Choose a reason for hiding this comment

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

One thing we should try to do if possible is use a root directory similar to here so that we don't have to define individual keys.

@damassi
Copy link
Member

damassi commented May 31, 2018

@l2succes - @zephraph pointed out that the reason decorators might be breaking with babel is because the legacy option might not be set. Can you try setting this in our babel config and see if switching from typescript-babel-jest to babel-jest fixes everything and all tests run?

tsconfig.json Outdated
}
},
"include": [
"node_modules/styled-components/typings/styled-components.d.ts",
Copy link
Member

Choose a reason for hiding this comment

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

Would moving this under `typeRoots be better? Like

"typeRoots": [
    "node_modules/@types",
    "node_modules/styled-components/typings/styled-components.d.ts"
  ]

(not sure)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I removed that line as it was just an experiment to fix TS errors

@l2succes
Copy link
Contributor Author

l2succes commented Jun 4, 2018

@damassi I can look into it but switching to ts-jest worked great.

I go through all of the type errors and verified .d.ts files are generating properly both with yarn compile and yarn watch

@damassi
Copy link
Member

damassi commented Jun 4, 2018

@l2succes - all good, was just curious if it compiled without errors, but nbd 👍

import Title from "./Title"
// @ts-ignore
import TextLink, { LinkProps } from "./TextLink"
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

@l2succes - I wonder if there is some way we could ignore imported interfaces without having to flag each unused import -- do you know if there is a TSLint / ts config option that would allow us to keep this clean?

Also, i'm pretty sure this is a shot in the dark, but any way to auto-export interfaces?

@orta / @alloy - have y'all ever come across a good way to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@damassi unfortunately, I don't think there's a way to ignore a whole block 😔 (Related: microsoft/TypeScript#19573)

They just fixed the main issue that enables us to emit types in the first place with typescript@2.9.1 since this issue is related I think they should give us a way to auto-import interfaces when generating types

Copy link
Member

Choose a reason for hiding this comment

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

Ahh cool cool, so I imagine we'll just have to track this as the feature becomes more mature 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Why are references being imported and then ignored?

Copy link
Member

Choose a reason for hiding this comment

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

@alloy - in order for the types to be automatically emitted, it needs an in-scope reference to the interface -- in the example above, LinkProps needs to be made available BUT because we're not using it directly in the code (only during the compilation process) it needs to be ignored due to the noUnusedLocals setting.

This aspect of type emission kinda bugs me because it clutters up code with some developer vs compiler indirection, but I suspect now that this feature has landed there will be some overall tooling improvements coming soon that will help clean the process up.

@l2succes
Copy link
Contributor Author

l2succes commented Jun 4, 2018

@artsy/engineering

We just upgraded typescript to 2.9 which allows us to enable emitting types for reaction. What that means is you'll see some new errors if you don't make certain things public. Some changes,
for any new components you work on, you'll need to export the Props interface associated with the component. For example:

export interface IconProps {
  name: string;
}

const Icon: React.SFC<IconProps> = props => {
  ...
}

@l2succes l2succes changed the title [WIP] Upgrades typescript and enables type emitting @damassi => Upgrades typescript and enables type emitting Jun 4, 2018
@damassi damassi merged commit b566045 into master Jun 4, 2018
@damassi
Copy link
Member

damassi commented Jun 4, 2018

@l2succes - Can you update README with some info on type emission and notify #dev since errors due to non-exported types could be confusing?

@yuki24 yuki24 deleted the emit-types branch June 4, 2018 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants