Skip to content
This repository has been archived by the owner on Mar 12, 2020. It is now read-only.

[WIP] Typescript conversion #438

Closed
wants to merge 28 commits into from
Closed

[WIP] Typescript conversion #438

wants to merge 28 commits into from

Conversation

l2succes
Copy link
Contributor

@l2succes l2succes commented Jan 30, 2017

This migrates the codebase from flow to typescript for type definitions.
Fixed just enough to get top level containers to run in the simulator.

You may need to run tsc --watch if files are not automatically being compiled by VS Code (didn't work for me but will fix).

To-do list:

  • Add types to remaining classes
  • Fix tests
  • Fix hot reloading

@ArtsyOpenSource
Copy link

ArtsyOpenSource commented Jan 30, 2017

Fails
🚫 No CHANGELOG added.

Generated by 🚫 dangerJS

@l2succes
Copy link
Contributor Author

@sarahscott I'm currently working on defining types and fixing errors for the Gene container and its sub-components. So feel free to work on any of the other containers. I should have that work pushed to this branch hopefully by EOD so you can be aligned on conventions.

@l2succes
Copy link
Contributor Author

@orta Sounds good, I added the hack for now but hot-reloading doesn't work yet so either I'll try to fix it or switch to using your fork.

@sarahscott
Copy link
Contributor

@l2succes fantastic! I'll start on the Artist container/components.

l2succes and others added 5 commits January 31, 2017 17:22
 - includes VS Code typescript related tasks
 - uses new @types modules instead of typings
Ran `node  ../relay2ts/dist/src/bin.js src/lib/**/*.(ts|tsx) --list
--update` to add interfaces for relay fragments.

Still need to add to `Props` for most components
render() {
const { children, ...props } = this.props
return (
<View>
<SwitchView style={styles.switch} {...props} />
Copy link
Contributor

Choose a reason for hiding this comment

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

This spread operator gave me a lot of trouble - I did appreciate the elegance of this class though so if someone knows how to pass the props in this way without ts errors, it'll be very appreciated 😄 .

}
}

class Article extends React.Component<Props, {}> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the second state type should be null if there’s no state.

tsconfig.json Outdated
"allowJs": true,
"sourceMap": true,
"moduleResolution": "node",
"strictNullChecks": true
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the good stuff

tsconfig.json Outdated
"build",
"index.ios.js"
],
"compileOnSave": true
Copy link
Contributor

Choose a reason for hiding this comment

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

this we want to eliminate via orta/systems-theory#1 and facebook/react-native#11932

const { View } = ReactNative;
const { TestModule } = ReactNative.NativeModules;
import Emission from 'emission'
import React from 'react'
Copy link
Contributor

@alloy alloy Feb 8, 2017

Choose a reason for hiding this comment

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

Shouldn’t this be?

import * as React from 'react'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's correct, still working on converting all the tests. I'll have it updated soon

Copy link
Contributor

Choose a reason for hiding this comment

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

nah, just checked, this doesn't need to change - it's still a JS file, and should stay one probably. It's only referenced by the iOS Sim Test Runner.

static propTypes = {
// A callback that is called when the user requests a retry.
onRetry: React.PropTypes.func,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You should move this to a Props interface instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to have propTypes defined for native components but yes this should be defined in the Props interface as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right, forgot about that 👍

This should probably be documented in the README:

  • JS components: only a TS interface (which inherits from x and y)
  • Native components: also add prop types

@alloy alloy mentioned this pull request Feb 23, 2017
7 tasks
@l2succes
Copy link
Contributor Author

Closing this PR in favour of the following one #453

@l2succes l2succes closed this Feb 24, 2017
@alloy alloy deleted the typescript branch March 24, 2017 12:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants