Introduce React, TypeScript, TSLint and React-StyleGuidist #2219

scottnonnenberg-signal · 2018-04-04T23:28:37Z

Quite a bit of change here.

First, the basics:

New dependencies were added: react, typescript, tslint, and react-styleguidist
A new npm script: transpile. It uses typescript to process .tsx files in js/react, putting .js files next to the original file. It's part of the watch functionality of grunt dev as well as the default task run with just grunt (used to build the app prior to release). A lighter-weight to get watch behavior when just working on React components is to run yarn transpile --watch.
yarn run clean-transpile will remove generated .js files

Style guide via react-styleguidist. Example site: https://react-styleguidist.js.org/examples/basic/

Start with yarn styleguide
Component.md files right next to the .tsx file
jsdoc-style comments are picked up and added to the generated part of the styleguide - the overall summary and a table listing methods and properties of the component
It has hot-reloading!
It uses webpack, which means that our app now pulls in webpack though we don't use it to generate anything for the production app.
I did a bunch of work to enable the use of Backbone views in this context, which will allow us to move smoothly from the old world to the new. First, add all the permutations in the old way, and then slowly start to re-render those same views with React.

A bit of dependency cleanup to enable use in React components:

moment was moved from our Bower dependencies to our npm dependencies, so it can be used in React components not running in a browser window.
i18n was moved into the new commonjs format, so it can be used in React components even if window is not available.

Lastly, a bit of Gruntfile cleanup:

Removal of Chrome App-era modifications of background.js
Make jshint/jscs watch more targeted, since more and more we'll be using other tools

npm run transpile Works on files under js/react/ Outputs files right next to the .tsx file This is part of our `grunt dev` task, as well as the default grunt task, which does everything else necessary to get a raw git checkout ready to run.

Due to a number of hacks, the style guide can be used to show Backbone views. This will allow a smooth path from the old way of doing things to the new.

We also fully set up i18n and put it on util as well as making it available on windows.i18n for Backbone views.

gasi-signal

First batch of comments. Super excited about this — nice work!!

Happy to discuss any details / style questions in person during standup 😄

gasi-signal · 2018-04-05T15:19:03Z

.eslintignore

 js/views/**/*.js
 test/*.js
 test/models/*.js
 test/views/*.js
 /*.js

+# typescript-generated files
+js/react/**/*.js


gasi-signal · 2018-04-05T15:19:17Z

.eslintignore

 # ES2015+ files
 !js/background.js
 !js/backup.js
 !js/database.js
 !js/logging.js
+!js/i18n.js


gasi-signal · 2018-04-05T15:20:24Z

Gruntfile.js

        '!js/modules/**/*.js',
        '!js/views/conversation_search_view.js',
+        '!js/react/**/*.js',


Minor: Could we try and keep 🔡 order in such lists to make them easier to scan and avoid duplicates? I often use Sublime Text‘s Sort Lines command 😄

gasi-signal · 2018-04-05T15:20:36Z

Gruntfile.js

-              return content;
-            }
-          }
-        }


gasi-signal · 2018-04-05T15:28:47Z

Gruntfile.js

        tasks: ['jshint']
      },
      style: {
-        files: ['<%= jscs.all.src %>', './js/**/*.js'],
+        files: ['<%= jscs.all.src %>'],


In the past, I’ve liked using git ls-files to only target files we are actually tracking but I know this can break down in places where we don’t have Git. In our case, we even have Git on CI though, right? Would you be open to that?

Not sure what you're suggesting here. We've already specified, in detail, what the jscs task applies to, so we'll be using that for the watch.

Apologies if I wasn’t clear. I meant that we often reference js/**/*.js but that includes all files in the repo, regardless of whether they’re tracked in Git or not. When working on a new feature, one can have JS files in the project that haven’t been checked in but still trigger lint errors, etc.

gasi-signal · 2018-04-05T15:43:26Z

js/react/conversation/Message.tsx

+      </li>
+    );
+  }
+}


gasi-signal · 2018-04-05T15:48:24Z

js/react/conversation/Message.tsx

+        <div className="bubble">
+          <div className="sender" dir="auto" />
+          <div className="attachments" />
+          <p className="content" dir="auto">


Curious: What’s our guideline for setting dir attribute? The following article suggests setting it where user enter data: https://www.w3.org/International/questions/qa-html-dir Do we set it here because our app is localized and the templates may be filled with either LTR or RTL text? I don’t have a lot of experience with bidi compatibility 😄

Pure history. Not really sure what the reasoning is there. I just copied what messages are generating today!

gasi-signal · 2018-04-05T15:52:46Z

js/react/util/BackboneWrapper.tsx

@@ -0,0 +1,78 @@
+import React from 'react';
+
+interface IProps {


Haha, this might be contentious but the TypeScript community has a convention not to use I prefixes for interfaces:

https://github.com/Microsoft/TypeScript/wiki/Coding-guidelines#names

Prohibition against prefixing interfaces with "I" microsoft/TypeScript-Handbook#121

Since we seem to agree to prefer not using classes, this shouldn’t be a big issue for us. Let’s discuss this in person 😄

Fixed! I only did it because tslint defaulted that way.

gasi-signal · 2018-04-05T15:54:42Z

js/react/util/BackboneWrapper.tsx

+      this.el = element;
+      this.setup();
+    };
+    this.setup = this.setup.bind(this);


Minor: If we use static initializers, we can avoid the manual binds (note the = assignment), e.g.

private handleClick = () => { this.handleUserLogin({isNew: false}) }

We used this extensively on previous team with great success! (we might have to enable some TSC option for it though, IIRC)

gasi-signal · 2018-04-05T15:55:57Z

js/react/util/MessageParents.tsx

+  /**
+   * Corresponds to the theme setting in the app, and the class added to the root element.
+   */
+  theme: 'ios' | 'android' | 'android-dark';


Yes, loving these union types!! http://www.typescriptlang.org/docs/handbook/advanced-types.html#union-types

gasi-signal

LGTM! Really excited about this — nice work and thanks for starting this 🎉

gasi-signal · 2018-04-05T17:05:57Z

js/react/util/BackboneWrapper.tsx

+    super(props);
+
+    this.el = null;
+    this.view = null;


Minor: You can initialize instance properties inline using static initializers:

protected el: Element | null = null; protected view: IBackboneView | null = null;

gasi-signal · 2018-04-05T17:13:45Z

js/react/util/index.ts

+
+// TypeScript wants two things when you import:
+//   1) a normal typescript file
+//   2) a javascript file with type definiitions


Minor typo: definiitions --> definitions

gasi-signal · 2018-04-05T17:14:04Z

js/react/util/MessageParents.tsx

+/**
+ * Provides the parent elements necessary to allow the main Signal Desktop stylesheet to
+ * apply (with no changes) to messages in this context.
+ */


Love this! Thanks for documenting.

gasi-signal · 2018-04-05T17:14:32Z

js/react/util/MessageParents.tsx

+ * Provides the parent elements necessary to allow the main Signal Desktop stylesheet to
+ * apply (with no changes) to messages in this context.
+ */
+export class MessageParents extends React.Component<IProps, {}> {


Minor: How about *Container or *Provider to align with Redux concepts?

gasi-signal · 2018-04-05T17:15:28Z

js/react/util/index.ts

+
+
+// Required, or TypeScript complains about adding keys to window
+const parent = window as any;


Grr, I hate when that happens. I think there are ways to define the new props but I’d have to do some research.

gasi-signal · 2018-04-05T17:24:42Z

styleguide.config.js

+        {
+          // To test handling of attachments, we need arraybuffers in memory
+          test: /\.(gif|mp3|mp4|txt)$/,
+          loader: 'arraybuffer-loader',


Nice find! 👀

(we were getting crashes in the initial setup of a backbone view)

gasi-signal

LGTM 👍 Just found one typo (I think)

gasi-signal · 2018-04-05T20:16:46Z

js/react/util/BackboneWrapper.md

@@ -1,4 +1,4 @@
-Rendering a real `Whisper.MessageView` using `<util.MessageParents />` and
+Rendering a real `Whisper.MessageView` using `<util.ConversationContextProvider />` and


Minor: ConversationContextProvider --> ConversationContext (I think)

gasi-signal · 2018-04-05T20:35:13Z

Last two commits LGTM 👍

Split out test-specific and general utility react components too. And moved our test/legacy* files for the Style Guide into a styleguide/ subdirectory of test/. I think we'll be able to live in this directory structure for a while.

gasi-signal

Looking good 🚢

gasi-signal · 2018-04-05T22:33:48Z

background.html

+  <!-- When making changes to these templates, be sure to update these two places:
+    1) test/styleguide/legacy_templates.js
+    2) test/index.html
+  -->


👏 👏 👏 Super helpful!

gasi-signal · 2018-04-05T22:34:16Z

test/styleguide/legacy_bridge.js

@@ -17,14 +18,14 @@ window.Signal.Migrations = {
  getPlaceholderMigrations: () => {},
 };

-window.Signal.React = window.Signal.React = {};
+window.Signal.React = {};


Reminds me - perhaps this should be window.Signal.Components?

gasi-signal · 2018-04-05T22:35:16Z

test/styleguide/legacy_templates.js


 window.Whisper.View.Templates = {
  hasRetry: `
-    {{ messageNotSent }}
-    <span href='#' class='retry'>{{ resend }}</span>
+    {{ messageNotSent }} <span href='#' class='retry'>{{ resend }}</span>


Oh, one thing you might notice is that since React doesn’t output spaces in the HTML, we might have to tweak certain paddings and margins in places! 😄 See: https://github.com/gasi/Signal-Desktop/pull/1/files#r165859574

These aren't rendered with React... yet. Duly noted, however!

gasi-signal

LGTM 👍

gasi-signal · 2018-04-05T23:13:46Z

ts/test/StyleGuideUtil.ts

-SignalReact.Message = Message;
-SignalReact.Reply = Reply;
+SignalComponents.Message = Message;
+SignalComponents.Reply = Reply;


Minor: What if we did the following?

parent.Signal.Components = { Message, Reply, };

Fixed: Conversation message preview would sometimes continue to show after message disappeared (1206b3c) Improve URL Auto-Linking In Messages (#2240) Redact More Variants Of Paths In Stack Traces (#2229) Dev: Introduce React, TypeScript, TSLint and React-StyleGuidist (#2219 and #2232)

Receive quoted replies (#2244) iOS theme: one bubble for both attachment and message contents (#2244) Improve URL Auto-Linking In Messages (#2240) Redact More Variants Of Paths In Stack Traces (#2229) Fixed: Conversation message preview would sometimes continue to show after message disappeared (1206b3c) Dev: Introduce React, TypeScript, TSLint and React-StyleGuidist (#2219 and #2232)

scottnonnenberg-signal added 12 commits April 4, 2018 15:56

Gruntfile: Remove edits to background.js; no longer applicable

26f69b6

Gruntfile: limit jshint/jscs watch tasks to own files, not *.js

e32dad2

Move moment from bower deps to npm deps

1ad2b08

Update style of i18n, pull it in via preload instead of .html

f86a6ef

Full styleguide now available via yarn styleguide

1326b26

Due to a number of hacks, the style guide can be used to show Backbone views. This will allow a smooth path from the old way of doing things to the new.

Add 'use strict' to i18n.js

3899eb8

Remove i18n.js from jshint run, fix missing semicolon

104a1ca

Add mp3 to project, make binary files available in rsg on util

ed12e1b

Add missing shim, fix typo

887abd7

Parse theme from querystring, put on util. Moment from require()

be91a89

We also fully set up i18n and put it on util as well as making it available on windows.i18n for Backbone views.

Fix tslint errors and add tslint to CI task in package.json

1052341

gasi-signal reviewed Apr 5, 2018

View reviewed changes

gasi-signal approved these changes Apr 5, 2018

View reviewed changes

scottnonnenberg-signal added 7 commits April 5, 2018 12:24

New tslint rule: Interfaces must not start with I

23293a3

Gruntfile: Sort lines in jshint/jscs config

bb73d47

Use Array.isArray in i18n.js

2384092

Fix typo in index.ts

61a7846

Rename test-only MessageParents to ConversationContext

cdd16c9

Finish rename of MessageParents, re-add moment to page

e5322e8

(we were getting crashes in the initial setup of a backbone view)

js/i18n.js -> js/modules/i18n.js

76be135

gasi-signal reviewed Apr 5, 2018

View reviewed changes

scottnonnenberg-signal added 2 commits April 5, 2018 13:42

Remove 'use strict' from i18n.js

50d4dba

gasi-signal approved these changes Apr 5, 2018

View reviewed changes

window.Signal.React -> window.Signal.Components

0530323

gasi-signal approved these changes Apr 5, 2018

View reviewed changes

Simplify assignment; add warning to preload.js about Style Guide

96bd90a

scottnonnenberg-signal merged commit c6c3b65 into development Apr 6, 2018

scottnonnenberg-signal deleted the react-ts-styleguide branch April 6, 2018 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce React, TypeScript, TSLint and React-StyleGuidist #2219

Introduce React, TypeScript, TSLint and React-StyleGuidist #2219

scottnonnenberg-signal commented Apr 4, 2018 •

edited

Loading

gasi-signal left a comment

gasi-signal Apr 5, 2018

gasi-signal Apr 5, 2018

gasi-signal Apr 5, 2018

gasi-signal Apr 5, 2018

gasi-signal Apr 5, 2018

scottnonnenberg-signal Apr 5, 2018

gasi Apr 9, 2018

gasi-signal Apr 5, 2018

gasi-signal Apr 5, 2018

scottnonnenberg-signal Apr 5, 2018

gasi-signal Apr 5, 2018

scottnonnenberg-signal Apr 5, 2018

gasi-signal Apr 5, 2018 •

edited

Loading

gasi-signal Apr 5, 2018

gasi-signal left a comment

gasi-signal Apr 5, 2018

gasi-signal Apr 5, 2018

gasi-signal Apr 5, 2018

gasi-signal Apr 5, 2018

scottnonnenberg-signal Apr 5, 2018

gasi-signal Apr 5, 2018

gasi-signal Apr 5, 2018

gasi-signal left a comment

gasi-signal Apr 5, 2018

gasi-signal commented Apr 5, 2018

gasi-signal left a comment

gasi-signal Apr 5, 2018

gasi-signal Apr 5, 2018

scottnonnenberg-signal Apr 5, 2018

gasi-signal Apr 5, 2018

scottnonnenberg-signal Apr 5, 2018

gasi-signal left a comment

gasi-signal Apr 5, 2018

		@@ -0,0 +1,78 @@
		import React from 'react';

		interface IProps {



		// Required, or TypeScript complains about adding keys to window
		const parent = window as any;

		@@ -1,4 +1,4 @@
		Rendering a real `Whisper.MessageView` using `<util.MessageParents />` and
		Rendering a real `Whisper.MessageView` using `<util.ConversationContextProvider />` and

Introduce React, TypeScript, TSLint and React-StyleGuidist #2219

Introduce React, TypeScript, TSLint and React-StyleGuidist #2219

Conversation

scottnonnenberg-signal commented Apr 4, 2018 • edited Loading

gasi-signal 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

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

gasi-signal Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gasi-signal 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

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

gasi-signal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gasi-signal commented Apr 5, 2018

gasi-signal 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gasi-signal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottnonnenberg-signal commented Apr 4, 2018 •

edited

Loading

gasi-signal Apr 5, 2018 •

edited

Loading