Shift plexus to TypeScript (from flowtypes) #331

tiffon · 2019-02-16T02:43:02Z

Partially addresses #306.

Outstanding items for this PR (before feedback):

Linting TypeScript in plexus
Document the build related changes
Misc cleanup
Resurrect the demo

Main outstanding items are to add linting to plexus TS files and more comments in the plexus build related files. Signed-off-by: Joe Farro <[email protected]>

Signed-off-by: Joe Farro <[email protected]>

codecov · 2019-02-16T02:52:33Z

Codecov Report

Merging #331 into master will increase coverage by 0.12%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master     #331      +/-   ##
==========================================
+ Coverage   83.12%   83.25%   +0.12%     
==========================================
  Files         145      145              
  Lines        3224     3224              
  Branches      658      660       +2     
==========================================
+ Hits         2680     2684       +4     
+ Misses        436      432       -4     
  Partials      108      108

Impacted Files	Coverage Δ
...ts/TracePage/TraceTimelineViewer/SpanTreeOffset.js	`100% <ø> (ø)`	⬆️
...ckages/jaeger-ui/src/components/common/CopyIcon.js	`100% <ø> (ø)`	⬆️
...jaeger-ui/src/components/App/TraceIDSearchInput.js	`0% <ø> (ø)`	⬆️
...kages/jaeger-ui/src/components/common/TraceName.js	`5.55% <ø> (ø)`	⬆️
.../jaeger-ui/src/components/DependencyGraph/index.js	`100% <ø> (ø)`	⬆️
...aceTimelineViewer/SpanDetail/AccordianKeyValues.js	`100% <ø> (ø)`	⬆️
...components/DependencyGraph/DependencyForceGraph.js	`84.21% <ø> (ø)`	⬆️
...omponents/TraceDiff/TraceDiffHeader/TraceHeader.js	`100% <ø> (ø)`	⬆️
...Page/TraceTimelineViewer/SpanDetail/DetailState.js	`94.44% <ø> (ø)`	⬆️
packages/jaeger-ui/src/utils/color-generator.js	`100% <ø> (ø)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update caf91a7...d346860. Read the comment docs.

everett980

This is a fantastic start. Some minor things came up but by and large it looks pretty good.
Three things came up that are a bit more open ended:
First, I haven't seen Enums used to define values before, but that seems to be working well here. I also haven't used Enums much before at all so maybe that is how they are intended to be used. Mostly curious if they should live in the type.tsx files if they are also constants? But I'm not sure what approach I would prefer over this.
Secondly, and this is something I'll look into more myself over the weekend, I am not sure when to use these three following type definitions:

key: Type | void;
or
key?: Type;
or
key?: Type | void;

Thirdly, if we want to be super thorough in type names, we may want to regex fix the type names that don't start with a T.

Then one other point is that we don't need to use ; instead of , in type definitions. I'm not overly attached to , over ; but do have a slight preference. If you do as well we may want to change but if not it's fine either way.

packages/jaeger-ui/.env

packages/jaeger-ui/config-overrides.js

packages/jaeger-ui/src/utils/DraggableManager/README.md

packages/plexus/.neutrinorc.js

packages/plexus/src/DirectedGraph/DirectedGraph.tsx

everett980 · 2019-02-22T21:16:04Z

packages/plexus/src/LayoutManager/Coordinator.tsx

@@ -17,33 +15,41 @@
 /* eslint-disable no-console */

 import * as convCoord from './dot/conv-coord';
-import workerFactory from './layout.worker';
+import LayoutWorker from 'worker/LayoutManager/layout.worker';


is this now from an npm package? if so I believe it should be the first import with a new line after.

Well, I needed to create a type definition for the layout.worker that reflected it being used by Coordinator.tsx as a WebWorker. In TypeScript, I don't think you can create auxiliary type defs for local files. So, I made an alias in webpack and then added a type def for the alias.

So, it's a local file loaded via an alias resolver.

Seems like it should be kept in the local files section of imports, but moved to the bottom.

packages/plexus/src/LayoutManager/Coordinator.tsx

everett980 · 2019-02-22T21:31:15Z

packages/plexus/src/LayoutManager/getLayout.tsx

 const VALIDITY_WARN = 'VALIDITY_WARN';
+const VALIDITY_ERROR = 'VALIDITY_ERROR';


not that important but seems like this should've moved up a line, not down

These were actually unused, so I remove them.

packages/plexus/src/LayoutManager/layout.worker.tsx

everett980 · 2019-02-22T21:44:11Z

packages/plexus/src/LayoutManager/layout.worker.tsx

+    meta: currentMeta,
+    errorMessage: {
+      colno,
+      error,


previously errorMessage.error was guaranteed to have the exactly the keys code, message, name, stack even if some may be not be present on event.data.error or if more are present on event.data.error
Not sure if this matters, but it may change the shape of this object.

The current code is more aligned with the spec. Not sure why I had the previous code, might have been from experimentation.

tiffon · 2019-02-22T22:18:11Z

Update

Contrary to the docs on void, when in strict mode, only undefined can be assigned to values of type void.

I've marked out the incorrect comments, below.

@everett980 Thanks for reviewing! Will check out your comments.

key: Type | void;
or
key?: Type;
or
key?: Type | void;

In the above, key: Type | void means the value will either be an instance of Type or ~~one of the values null or~~ undefined. It's the equivalent of key: Type | undefined ~~key: ?Type in flow~~.

key?: Type means the presence of key is optional.

key?: Type | void is a combo of the two: the presence of key is optional and if present it can be either a Type instance or ~~null or~~ undefined.

Thirdly, if we want to be super thorough in type names, we may want to regex fix the type names that don't start with a T.

Agreed. Turns out I missed a few!

Then one other point is that we don't need to use ; instead of , in type definitions. I'm not overly attached to , over ; but do have a slight preference. If you do as well we may want to change but if not it's fine either way.

I think we should use ; because that's the style used by the TS team.

This required an update to `eslint-plugin-import`, which, in turn, emtailed updating a few packages. This resulted in the addition of a few new rules. So, there are also changes to adhere to the new rules. Signed-off-by: Joe Farro <[email protected]>

Signed-off-by: Joe Farro <[email protected]>

TypeScript linting is not quite correctly intergrated with VS code, but the current eslint and tsc-lint npm scripts have TS linting in pretty good shape. tsc-lint endeavors to build the TS files but does not emit them, thereby filling in the gaps left by eslint TS support. Signed-off-by: Joe Farro <[email protected]>

Signed-off-by: Joe Farro <[email protected]>

everett980 · 2019-02-28T16:32:13Z

typings/index.d.ts

-  colno?: number,
-  error?: any,
-};
+/// <reference path="custom.d.ts" />


Does this file serve a purpose if it is just comments?

Hey, thought I responded to this. Yes, it does. Added docs for this, here:

https://github.com/jaegertracing/jaeger-ui/pull/331/files#diff-c21202b241d0cd40822db520c8b00770R103

- Main type exports moved from src/types/layout to src/types/index - Change worker alias to be more obvious, e.g. "worker-alias" - Various props made optional on: - DirectedGraph - PureEdges - PureNodes Signed-off-by: Joe Farro <[email protected]>

Signed-off-by: Joe Farro <[email protected]>

everett980

LGTM. Left some minor comments, most of which have to do with creating issues for future work.

BUILD.md

packages/plexus/BUILD.md

packages/plexus/demo/src/index.tsx

Signed-off-by: Joe Farro <[email protected]>

* Shift plexus to TypeScript (from flowtypes) Main outstanding items are to add linting to plexus TS files and more comments in the plexus build related files. Signed-off-by: Joe Farro <[email protected]> * Typo from renaming Signed-off-by: Joe Farro <[email protected]> * Add linting for plexus TypeScript This required an update to `eslint-plugin-import`, which, in turn, emtailed updating a few packages. This resulted in the addition of a few new rules. So, there are also changes to adhere to the new rules. Signed-off-by: Joe Farro <[email protected]> * Fix a few issues from merging master Signed-off-by: Joe Farro <[email protected]> * Better linting for plexus / TypeScript TypeScript linting is not quite correctly intergrated with VS code, but the current eslint and tsc-lint npm scripts have TS linting in pretty good shape. tsc-lint endeavors to build the TS files but does not emit them, thereby filling in the gaps left by eslint TS support. Signed-off-by: Joe Farro <[email protected]> * Fix lint error from merging master Signed-off-by: Joe Farro <[email protected]> * TypeScript type cleanup Signed-off-by: Joe Farro <[email protected]> * Build docs for root and plexus, revive plexus demo - Main type exports moved from src/types/layout to src/types/index - Change worker alias to be more obvious, e.g. "worker-alias" - Various props made optional on: - DirectedGraph - PureEdges - PureNodes Signed-off-by: Joe Farro <[email protected]> * Update changelog Signed-off-by: Joe Farro <[email protected]> * Misc cleanup and add GitHub tickets to build docs Signed-off-by: Joe Farro <[email protected]> * Add license to package.json for publishing Signed-off-by: Joe Farro <[email protected]> Signed-off-by: vvvprabhakar <[email protected]>

Shift plexus to TypeScript (from flowtypes)

a8a9175

Main outstanding items are to add linting to plexus TS files and more comments in the plexus build related files. Signed-off-by: Joe Farro <[email protected]>

tiffon requested a review from everett980 February 16, 2019 02:43

ghost assigned tiffon Feb 16, 2019

ghost added the review label Feb 16, 2019

Typo from renaming

dda5e5c

Signed-off-by: Joe Farro <[email protected]>

everett980 requested changes Feb 22, 2019

View reviewed changes

Merge branch 'master' into plexus-to-ts

e523600

tiffon added 6 commits February 24, 2019 17:23

Add linting for plexus TypeScript

ccf11d1

This required an update to `eslint-plugin-import`, which, in turn, emtailed updating a few packages. This resulted in the addition of a few new rules. So, there are also changes to adhere to the new rules. Signed-off-by: Joe Farro <[email protected]>

Fix a few issues from merging master

bfd29f2

Signed-off-by: Joe Farro <[email protected]>

Merge branch 'master' into plexus-to-ts

7014c68

Fix lint error from merging master

483d7b1

Signed-off-by: Joe Farro <[email protected]>

TypeScript type cleanup

5de93e0

Signed-off-by: Joe Farro <[email protected]>

everett980 approved these changes Feb 28, 2019

View reviewed changes

everett980 reviewed Feb 28, 2019

View reviewed changes

tiffon and others added 3 commits March 7, 2019 11:00

Merge branch 'master' into plexus-to-ts

82788a0

Update changelog

b349003

Signed-off-by: Joe Farro <[email protected]>

everett980 reviewed Mar 7, 2019

View reviewed changes

tiffon added 2 commits March 8, 2019 15:05

Misc cleanup and add GitHub tickets to build docs

cf3dd60

Signed-off-by: Joe Farro <[email protected]>

Add license to package.json for publishing

d346860

Signed-off-by: Joe Farro <[email protected]>

tiffon merged commit 2a179bf into master Mar 8, 2019

ghost removed the review label Mar 8, 2019

yurishkuro deleted the plexus-to-ts branch January 29, 2020 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shift plexus to TypeScript (from flowtypes) #331

Shift plexus to TypeScript (from flowtypes) #331

tiffon commented Feb 16, 2019 •

edited

Loading

codecov bot commented Feb 16, 2019 •

edited

Loading

everett980 left a comment

everett980 Feb 22, 2019

tiffon Feb 25, 2019

everett980 Feb 22, 2019

tiffon Feb 25, 2019

everett980 Feb 22, 2019

tiffon Feb 25, 2019

tiffon commented Feb 22, 2019 •

edited

Loading

everett980 Feb 28, 2019

tiffon Mar 7, 2019

everett980 left a comment

		const VALIDITY_WARN = 'VALIDITY_WARN';
		const VALIDITY_ERROR = 'VALIDITY_ERROR';

Shift plexus to TypeScript (from flowtypes) #331

Shift plexus to TypeScript (from flowtypes) #331

Conversation

tiffon commented Feb 16, 2019 • edited Loading

codecov bot commented Feb 16, 2019 • edited Loading

Codecov Report

everett980 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

tiffon commented Feb 22, 2019 • edited Loading

Update

Choose a reason for hiding this comment

Choose a reason for hiding this comment

everett980 left a comment

Choose a reason for hiding this comment

tiffon commented Feb 16, 2019 •

edited

Loading

codecov bot commented Feb 16, 2019 •

edited

Loading

tiffon commented Feb 22, 2019 •

edited

Loading