Convert codebase to Typescript #114

alex3165 · 2017-02-03T23:35:54Z

This PR include the following changes:

Convert all source files to typescript
Convert the test to typescript using ts-jest as a preprocessor

I would need help to review it, mainly the source files and have a look at the typescript interfaces.

PS: Examples are still written in ES6 compiled with Babel

alex3165 · 2017-02-06T22:09:57Z

@z0d14c @philpl @VilliGunn @ericrwolfe @arthur31416 Hey guys, as you are the most actives on the project I would like to ask you if you can have a look at this PR.

Sorry the PR is quite big

z0d14c · 2017-02-07T06:30:15Z

Hi, I didn't take a fine-toothed comb to the PR but it looks good from a bird's-eye view.

edit: As a further detail: I am not a typescript expert. That being said, I thoroughly support introducing typing to the project. The one caveat I would add is that if the addition of TS significantly changes development (probably yes) or installation (hopefully not) of the project, can we please document that in the README. :)

Wykks · 2017-02-07T13:26:48Z

tsconfig.json

+  "compilerOptions": {
+    "outDir": "lib",
+    "module": "commonjs",
+    "target": "es6",


Removing babel and setting ts target to es6 means that you don't support non es6 compatible browser.

You may want to set this to es5 but be aware that ts doesn't transpile es6 to es5 out of the box (e.g microsoft/TypeScript#11209).
In case of errors, you may need to reintroduce babel after tsc.

@Wykks Yep, totally correct, except TS does compile ES6 to ES5, except for very specific staged things like: Iterables / Iterators, Async / Await & Generators

Yeah that's what I meant :)
(And it will be fixed soon microsoft/TypeScript#12346)

I have changed the target to ES5, here is the commit: ec97fb7

calocan · 2017-02-07T21:40:51Z

The pull request doesn't seem to build, something isn't parsing in feature.ts for me

 npm run lint && npm run test && tsc


> [email protected] lint /Users/andy/code/react-mapbox-gl
> tslint --project tsconfig.json


> [email protected] test /Users/andy/code/react-mapbox-gl
> jest

 PASS  src/__tests__/layer.test.tsx
  Layer
    ✓ Should render layer with default options (16ms)
    ✓ Should render layer with default source (2ms)

Test Suites: 1 passed, 1 total
Tests:       2 passed, 2 total
Snapshots:   0 total
Time:        1.397s
Ran all test suites.
src/feature.ts(11,7): error TS2322: Type '() => null' is not assignable to type 'StatelessComponent<Props>'.
  Type 'null' is not assignable to type 'ReactElement<any>'.

npm ERR! Darwin 15.6.0
npm ERR! argv "/Users/andy/.nvm/versions/node/v7.3.0/bin/node" "/Users/andy/.nvm/versions/node/v7.3.0/bin/npm" "run" "build"
npm ERR! node v7.3.0
npm ERR! npm  v4.1.2
npm ERR! code ELIFECYCLE
npm ERR! [email protected] build: `npm run lint && npm run test && tsc`
npm ERR! Exit status 2
npm ERR! 
npm ERR! Failed at the [email protected] build script 'npm run lint && npm run test && tsc'.
npm ERR! Make sure you have the latest version of node.js and npm installed.
npm ERR! If you do, this is most likely a problem with the react-mapbox-gl package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     npm run lint && npm run test && tsc
npm ERR! You can get information on how to open an issue for this project with:
npm ERR!     npm bugs react-mapbox-gl
npm ERR! Or if that isn't available, you can get their info via:
npm ERR!     npm owner ls react-mapbox-gl
npm ERR! There is likely additional logging output above.

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/andy/code/react-mapbox-gl/npm-debug.log

npm WARN [email protected] requires a peer of jest@^18.0.0 but none was installed.
npm ERR! Darwin 15.6.0
npm ERR! argv "/Users/andy/.nvm/versions/node/v7.3.0/bin/node" "/Users/andy/.nvm/versions/node/v7.3.0/bin/npm" "install"
npm ERR! node v7.3.0
npm ERR! npm  v4.1.2
npm ERR! code ELIFECYCLE
npm ERR! [email protected] prepublish: `npm run clean && npm run build`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the [email protected] prepublish script 'npm run clean && npm run build'.
npm ERR! Make sure you have the latest version of node.js and npm installed.
npm ERR! If you do, this is most likely a problem with the react-mapbox-gl package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     npm run clean && npm run build
npm ERR! You can get information on how to open an issue for this project with:
npm ERR!     npm bugs react-mapbox-gl
npm ERR! Or if that isn't available, you can get their info via:
npm ERR!     npm owner ls react-mapbox-gl
npm ERR! There is likely additional logging output above.

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/andy/code/react-mapbox-gl/npm-debug.log

Wykks

Just read cluster.tsx for now, hope this help :)

Wykks · 2017-02-07T20:45:14Z

package.json

+    "@types/react": "^15.0.6",
+    "@types/react-addons-test-utils": "^0.14.17",
+    "@types/recompose": "^0.20.3",
+    "@types/typescript": "^2.0.0",


Use typescript instead of @types/typescript

Wykks · 2017-02-07T20:51:27Z

src/cluster.tsx

+const supercluster = require('supercluster'); //tslint:disable-line
+
+export interface Props {
+  ClusterMarkerFactory: (coordinates: number[], pointCount: number) => JSX.Element;


You can write this like this :
ClusterMarkerFactory(coordinates: number[], pointCount: number): JSX.Element;
(tslint rule : https://palantir.github.io/tslint/rules/prefer-method-signature/)

What do you think about it?

Wykks · 2017-02-07T20:53:31Z

src/cluster.tsx

+  extent?: number;
+  nodeSize?: number;
+  log?: boolean;
+  children?: Array<React.Component<MarkerProps, void>>;


Use either Array<T> or T[]
You should set https://palantir.github.io/tslint/rules/array-type/

I am using array-simple already but React.Component<MarkerProps, void> is not a simple type

Okay make sense, missed array-simple.
Although, I'm not sure if mixing two different syntax for the same purpose is a good idea.

Wykks · 2017-02-07T20:58:00Z

src/cluster.tsx

@@ -0,0 +1,136 @@
+import * as React from 'react';
+import * as MapboxGL from 'mapbox-gl';


Importing MapboxGL this way, add a hard dependency to Mapboxgl in this file, while it's not necessary.
You just need the type, you can use the ambient definition mapboxgl instead.
Or you don't want to use ambient definition ? (which is understandable)

if by using the ambient definition you mean this

/// <renference type="..."/>

I prefer not to use it because if i understand it well it expose globaly all the types of your reference in your project using this right ? even though I think it is difficult to track where the type come from

Not exactly, with @ types, every types inside the @ type directory is already loaded. So you can already use mapboxgl as is.
But sometimes, namespace are not exposed globally and you have to use the import for type definition...
Also typescript does elide this kind of import but I'm not sure exactly when (microsoft/TypeScript#2812)
Since I'm not sure about how typescript transpile this, I personnally perfer to not use import when it's not necessary.

Wykks · 2017-02-07T21:13:41Z

src/cluster.tsx

+import * as MapboxGL from 'mapbox-gl';
+import { Props as MarkerProps } from './marker';
+
+const supercluster = require('supercluster'); //tslint:disable-line


Here require is used to get around noImplicitAny(and tslint).
I think it's better to write something like this (in a global.d.ts file):

declare module 'supercluster' { const supercluster: any; export default supercluster; }

(Repeat this comment for each require like this)

done 2ea4e37

Wykks · 2017-02-07T21:14:47Z

package.json

-    "eslint-plugin-react": "^6.5.0",
+    "@types/jest": "^18.1.1",
+    "@types/mapbox-gl": "^0.29.0",
+    "@types/node": "^7.0.5",


If you remove every require you don't need this anymore (+ it's a bit overkill to add node types definition just for require)

Wykks · 2017-02-07T21:39:33Z

src/cluster.tsx

+    }
+  }
+
+  private feature(coordinates: number[]) {


private feature(coordinates: GeoJSON.Position): GeoJSON.Feature<GeoJSON.Point>

(You can add import * as GeoJSON from 'geojson' instead of using ambient type def)

GeoJSON is added by @types/mapbox-gl but you should explicitly add it too since you're using it

Since GeoJSON.Feature is not right I have done this: private feature(coordinates: GeoJSON.Position): Point

Wykks · 2017-02-07T21:43:24Z

src/cluster.tsx

+  map: MapboxGL.Map;
+}
+
+export interface Point {


Remove this, use GeoJSON.Feature<GeoJSON.Point> instead

The type definition for GeoJSON.Feature is fucked : properties: {} | null; and should be properties?: {[key: string]: any}

Where is the issue exactly ?
I don't think that properties should be optional (http://geojson.org/geojson-spec.html#feature-objects)

The problem is not optional it is {} which mean empty object so you cannot pass any property

Indeed, that's quite new.
You can use the previous version of @types/geojson then
"@types/geojson": "0.0.32"

Wykks · 2017-02-07T21:43:47Z

src/cluster.tsx

+    return (
+      <div>
+        {// tslint:disable-line
+          clusterPoints.map(({ geometry, properties }: Point) => (


clusterPoints.map(({ geometry, properties }: GeoJSON.Feature<GeoJSON.Point>)

same GeoJSON.Feature is not right

…ntime

alex3165 · 2017-02-08T21:53:29Z

@calocan Thanks for the catch, it should compile correctly now

kitten · 2017-02-09T16:12:39Z

src/cluster.tsx

@@ -0,0 +1,138 @@
+import * as React from 'react';
+import * as MapboxGL from 'mapbox-gl';


This thing will totally destroy tree-shaking. Same goes for * as React. Don't do that in libraries ;)

kitten · 2017-02-09T16:13:37Z

src/cluster.tsx

+  extent?: number;
+  nodeSize?: number;
+  log?: boolean;
+  children?: Array<React.Component<MarkerProps, void>>;


Consistency, I have to agree here. It's Array<> here and sometimes x[] below

kitten · 2017-02-09T16:14:00Z

src/cluster.tsx

+}
+
+export interface State {
+  superC: any;


Lots of anys... Is that a good thing?

I agree, supercluster should be (manually) typed since it cause too much any.

kitten · 2017-02-09T16:14:46Z

src/cluster.tsx

+    map: React.PropTypes.object
+  };
+
+  public static defaultProps = {


Pretty sure that you don't need to say that a static variable is public, since private/protected static shouldn't be a thing

kitten · 2017-02-09T16:14:59Z

src/cluster.tsx

+    const { children } = this.props;
+
+    if (children) {
+      const features = this.childrenToFeatures(children as any);


kitten · 2017-02-09T16:27:22Z

src/marker.tsx

+  public render() {
+    return (
+      <ProjectedLayer
+        {...{ ...this.props, children: undefined}}


This isn't nice to read... Also it's unnecessary. 1) It might not compile to just createElement(ProjectedLayer, this.props), but if it does, that'd be fine, because immutability is guaranteed on these objects.

Also, this could be a SFC

kitten · 2017-02-09T16:28:57Z

src/popup.tsx

+  public render() {
+    return (
+      <ProjectedLayer
+        {...{ ...this.props, children: undefined }}


kitten · 2017-02-09T16:29:22Z

src/projected-layer.tsx

-    map: PropTypes.object,
-  };
+export interface Props {
+  coordinates: number[];


Same: number[][]

Actually... Since it's always a tuple: [number, number][]

kitten · 2017-02-09T16:30:07Z

src/scale-control.tsx

    const { measurement } = this.props;

-    const clientWidth = map._canvas.clientWidth;
-    const { _ne, _sw } = map.getBounds();
+    const clientWidth = (map as any)._canvas.clientWidth;


kitten · 2017-02-09T16:30:42Z

src/source.ts

+      (map
+        .getSource(id) as GeoJSONSource)
+        .setData((props.sourceOptions as GeoJSONSourceRaw).data as any);
+    }


It's a bit tricky here.
@alex3165
First, simplify sourceOptions type. You don't really need this :

export type Sources = VectorSource | RasterSource | GeoJSONSource | ImageSource | VideoSource | GeoJSONSourceRaw;

Just do this :
sourceOptions: GeoJSONSourceRaw;

And then you can write this line like this :

if (props.sourceOptions.data !== sourceOptions.data) { const source = map.getSource(id) as GeoJSONSource; const data = props.sourceOptions.data as GeoJSON.Feature<GeoJSON.GeometryObject> | GeoJSON.FeatureCollection<GeoJSON.GeometryObject> | string; source.setData(data); }

By the way the long type GeoJSON.Feature<GeoJSON.GeometryObject> | GeoJSON.FeatureCollection<GeoJSON.GeometryObject> | string is also present in geojson-layer.ts, so you can make a type alias of this.

Wykks · 2017-02-09T22:47:57Z

src/constants/css.ts

@@ -1,3 +1,4 @@
+// tslint:disable


It's better to disable the concerned rule instead of disabling everything:
// tslint:disable:max-line-length

done 61cd6d6

Wykks · 2017-02-09T23:51:21Z

tsconfig.json

+    "noImplicitThis": true,
+    "noImplicitAny": true,
+    "strictNullChecks": true,
+    "suppressImplicitAnyIndexErrors": true,


Kill it with 🔥
Seriously this allow stuff like :

type obj = { stuff: string }; const foo: obj = {stuff: ''}; const data = 'notstuff'; const bar = foo[data];

This option is old, now we can use keyof for better typing:

type obj = { stuff: string }; const foo: obj = {stuff: ''}; const data: keyof obj = 'notstuff'; // Type '"notstuff"' is not assignable to type '"stuff"' const bar = foo[data];

done 9bb5f83

alex3165 mentioned this pull request Feb 3, 2017

Convert the project to typescript #40

Closed

alex3165 added 14 commits February 5, 2017 20:01

Setup typescript and start converting component

9da425e

Convert all files to ts

082a0b8

Add missing files

a14c00a

Convert popup and map to typescript, start layer

d6f54f4

Convert all components

b9d1209

Fix overlays

777b42c

Fix all typings

bab35ca

Add declaration files to build

e67284d

Move source to ts

1a6ce24

Add correct configuration

dadd1d9

Fix small error and add build wathc

d05f8de

Fix tests and lint

a86fc7d

Fix scale control

0186822

Fix from merge of source update

1bd8dc5

alex3165 force-pushed the typescript branch from 1d168c3 to 1bd8dc5 Compare February 5, 2017 20:09

alex3165 added 2 commits February 7, 2017 09:17

Improve readme file

db173f6

Improve readme again

5ba2483

alex3165 mentioned this pull request Feb 7, 2017

running tests with react-mapbox-gl installed #116

Closed

Wykks self-requested a review February 7, 2017 13:21

Wykks suggested changes Feb 7, 2017

View reviewed changes

alex3165 added 2 commits February 8, 2017 21:18

Target es5 and switch from find to filter

ec97fb7

Fix typescript, fix feature geojson object to the standard in the mea…

d78a016

…ntime

alex3165 added 2 commits February 8, 2017 23:12

Switch to real typescript package

425de49

Remove node types and introduce custom any types

2ea4e37

alex3165 added 2 commits February 8, 2017 23:32

Fix lint

8b8fb1d

Fix some types in cluster

9cc651e

kitten suggested changes Feb 9, 2017

View reviewed changes

Wykks reviewed Feb 9, 2017

View reviewed changes

alex3165 added 10 commits February 10, 2017 10:47

Fix coordinates typings

2d40152

Killed suppressImplicitAnyIndexErrors with 🔥

9bb5f83

Fix css.ts

61cd6d6

Disable multiline only in cluster

5632416

Remove missing suppressImplicitAnyIndexErrors

bd023a8

Add back suppressImplicitAnyIndexErrors

563ccd1

Fix remaining type issues

fca4a09

Fix deep-equal

6263da1

Fix critical types

fada71a

Improve readme

74118f5

alex3165 merged commit 768679b into master Feb 13, 2017

alex3165 deleted the typescript branch February 23, 2017 09:05

		@@ -0,0 +1,136 @@
		import * as React from 'react';
		import * as MapboxGL from 'mapbox-gl';

		@@ -0,0 +1,138 @@
		import * as React from 'react';
		import * as MapboxGL from 'mapbox-gl';

Convert codebase to Typescript #114

Convert codebase to Typescript #114

Conversation

alex3165 commented Feb 3, 2017 • edited Loading

alex3165 commented Feb 6, 2017

z0d14c commented Feb 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calocan commented Feb 7, 2017

Wykks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wykks Feb 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wykks Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wykks Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wykks Feb 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex3165 Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

Wykks Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex3165 commented Feb 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wykks Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

Wykks Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wykks Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex3165 commented Feb 3, 2017 •

edited

Loading

z0d14c commented Feb 7, 2017 •

edited

Loading

Wykks Feb 7, 2017 •

edited

Loading

Wykks Feb 9, 2017 •

edited

Loading

Wykks Feb 9, 2017 •

edited

Loading

Wykks Feb 7, 2017 •

edited

Loading

alex3165 Feb 8, 2017 •

edited

Loading

Wykks Feb 9, 2017 •

edited

Loading

Wykks Feb 9, 2017 •

edited

Loading

Wykks Feb 9, 2017 •

edited

Loading

Wykks Feb 9, 2017 •

edited

Loading