Enabling various Typescript Compile Options (strict null checks, etc.) #280

deregtd · 2017-09-18T20:53:35Z

```
   "noImplicitReturns": true,
```
```
   "noImplicitThis": true,
```
```
   "noUnusedLocals": true,
```
```
   "strictNullChecks": true,
```

   "forceConsistentCasingInFileNames": true,

msftclas · 2017-09-18T20:53:38Z

@deregtd,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

ms-markda · 2017-09-18T20:58:35Z

src/common/Types.ts

-    type: React.ComponentClass<P>;
-    props: P;
-}
+// interface Element<P> {


Why not remove?

It was probably wanted by someone originally. Gets compiled away.

ms-markda · 2017-09-18T20:58:48Z

src/macos/Input.ts

 */

-import RN = require('react-native');
+//import RN = require('react-native');


Why not remove?

When they build this component, they'll want this line back.

erictraut · 2017-09-18T20:58:07Z

src/android/Text.tsx

-    protected _getStyles(): Types.TextStyleRuleSet | Types.TextStyleRuleSet[] {
-        return Styles.combine<Types.TextStyle>([_styles.defaultText, this.props.style]);
+    protected _getStyles(): Types.StyleRuleSetRecursiveArray<Types.TextStyleRuleSet> {
+        return _.compact([_styles.defaultText, this.props.style]);


Why are you calling compact here? It's fine to have undefined or null values in the styles that are sent to RN.

erictraut · 2017-09-18T21:01:59Z

src/common/Types.ts

    title?: string;

-    onLoad?: (size: Dimensions) => void;
+    onLoad?: (size: OptionalDimensions) => void;


Why are these fields optional? Are there circumstances where RN can't hand back valid dimensions? I'd prefer if these are non-optional. Otherwise it complicates the logic in all components that use this callback.

erictraut · 2017-09-18T21:02:37Z

src/common/Types.ts

    // IMPORTANT NOTE: This handler may be called when the component is
    // already unmounted as it uses a time delay to accommodate a fade-out animation.
-    onAnchorPressed?: (e: RX.Types.SyntheticEvent) => void;
+    onAnchorPressed?: (e?: RX.Types.SyntheticEvent) => void;


Why is this parameter optional? Seems like it should be required.

talked offline

erictraut · 2017-09-18T21:03:01Z

src/common/Types.ts

-    type: React.ComponentClass<P>;
-    props: P;
-}
+// interface Element<P> {


If this is unused, you can delete it.

erictraut · 2017-09-18T21:03:24Z

src/common/Types.ts

    width: number;
    height: number;
 };
+export type OptionalDimensions = {


See note above. I don't think these fields should be optional.

erictraut · 2017-09-18T21:11:12Z

src/native-common/GestureView.tsx


        return (timeStamp - initialTimeStamp <= _tapDurationThreshold &&
-            this._calcDistance(initialPageX - e.pageX, initialPageY - e.pageY) <= _tapPixelThreshold);
+            this._calcDistance(initialPageX - (e.pageX || 0), initialPageY - (e.pageY || 0)) <= _tapPixelThreshold);


For these types of events, we're guaranteed that pageX and pageY are defined. So you could keep the code as is or change it to e.pageX!!!, etc.

erictraut · 2017-09-18T21:21:48Z

src/web/AlertModalContent.tsx

                        onHoverStart={ () => this.setState({ hoverIndex: i }) }
                        onHoverEnd={ () => this.setState({ hoverIndex: -1 }) }
-                        style={ buttonStyle }
+                        style={ _.compact(buttonStyle) }


Why are you compacting these styles? It's perfectly legit for style arrays to contain undefined values.

erictraut · 2017-09-18T21:25:06Z

src/web/GestureView.tsx


    private _getPanPixelThreshold = () => {
-        return this.props.panPixelThreshold > 0 ? this.props.panPixelThreshold : _panPixelThreshold;
+        return (this.props.panPixelThreshold && this.props.panPixelThreshold > 0) ? this.props.panPixelThreshold : _panPixelThreshold;


Zero values work OK in this logic, but it would be better if the check for undefined was more explicit.

erictraut · 2017-09-18T21:27:17Z

src/web/RootView.tsx

        if (this.props.autoDismiss) {
            // Should we immediately hide it, or did the caller request a delay?
-            if (this.props.autoDismissDelay > 0) {
+            if (this.props.autoDismissDelay && this.props.autoDismissDelay > 0) {


Zero values work OK with this logic, but an explicit undefined type would be better.

erictraut · 2017-09-18T21:28:22Z

src/web/Storage.ts

    getItem(key: string): SyncTasks.Promise<string|undefined> {
        const value = window.localStorage.getItem(key);
-        return SyncTasks.Resolved<string|undefined>(value);
+        return SyncTasks.Resolved<string|undefined>(value || undefined);


Why do you want to turn empty strings into undefined here? I don't think that's the right semantic.

ms-markda

Nothing seems out of place with respect to the TypeScript transformations. The interface changes seem alright, but you might want another opinion on that.

…m. Made the exported reactxp.ts files per-platform export types from the base classes instead of their specific implementation. This brought a bunch of interface holes to the surface.

…nitions (microsoft#280) * Rename microsoft#1 for ViewBase to fix caps * Rename microsoft#2 to fix ViewBase. Enabling forceConsistentCasingInFileNames. * Enabling noImplicitReturns, fixing a bug in native-common/Linking.ts * Enabling noImplicitThis * Enabling noUnusedLocals and fixing some bugs it found * Enabling strict null checks on reactxp * Addressing PR feedback * Fixing a bunch of type exporting/checking issues * Fixed a bunch of base interfaces and made everything inherit from them. Made the exported reactxp.ts files per-platform export types from the base classes instead of their specific implementation. This brought a bunch of interface holes to the surface.

David de Regt added 6 commits September 18, 2017 11:44

Rename #1 for ViewBase to fix caps

2108cb7

Rename #2 to fix ViewBase. Enabling forceConsistentCasingInFileNames.

aa15d5a

Enabling noImplicitReturns, fixing a bug in native-common/Linking.ts

80c7f27

Enabling noImplicitThis

26b18a8

Enabling noUnusedLocals and fixing some bugs it found

dadc49e

Enabling strict null checks on reactxp

bb06b99

deregtd requested review from berickson1, erictraut and ms-markda September 18, 2017 20:53

msftclas added the cla-not-required label Sep 18, 2017

ms-markda reviewed Sep 18, 2017

View reviewed changes

erictraut reviewed Sep 18, 2017

View reviewed changes

Addressing PR feedback

5f3c18c

ms-markda approved these changes Sep 18, 2017

View reviewed changes

David de Regt added 2 commits September 18, 2017 16:54

Fixing a bunch of type exporting/checking issues

0fcb7eb

Fixed a bunch of base interfaces and made everything inherit from the…

4099766

…m. Made the exported reactxp.ts files per-platform export types from the base classes instead of their specific implementation. This brought a bunch of interface holes to the surface.

deregtd merged commit fdccce2 into master Sep 19, 2017

deregtd deleted the dadere/TSOptions branch September 19, 2017 04:47

Enabling various Typescript Compile Options (strict null checks, etc.) #280

Enabling various Typescript Compile Options (strict null checks, etc.) #280

Uh oh!

Conversation

deregtd commented Sep 18, 2017

Uh oh!

msftclas commented Sep 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ms-markda left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants