Type annotations #4789

trusktr · 2022-01-26T19:44:44Z

closes #4793

Describe your changes:

mostly type annotations
some small code improvements to make types work better, removal of unused args, and small cleanups

All Submissions:

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?
Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots.

Author Checklist

Changes address original issue?
Unit tests included and/or updated with changes?
Command line build passes?
Has this been smoke tested?
Testing instructions included in associated issue?

Reviewer Checklist

Changes appear to address issue?
Changes appear not to be breaking changes?
Appropriate unit tests included?
Code style and in-line documentation are appropriate?
Commit messages meet standards?
Has associated issue been labelled unverified? (only applicable if this PR closes the issue)
Has associated issue been labelled bug? (only applicable if this PR is for a bug fix)

… plots

src/plugins/plot/configuration/XAxisModel.js

trusktr · 2022-01-26T19:54:34Z

package.json

@@ -7,6 +7,8 @@
    "@percy/cli": "^1.0.0-beta.73",
    "@percy/playwright": "^1.0.1",
    "@playwright/test": "^1.18.0",
+    "@types/eventemitter3": "^1.0.0",
+    "@types/lodash": "^4.14.178",


This doesn't update the packages we're currently using, this only adds types (hence the @types/) of the package's we're using which gives us intellisense in IDEs.

trusktr · 2022-01-26T19:58:05Z

src/plugins/plot/chart/MCTChartSeriesElement.js

+        // TODO useless makePoint call?
+        this.makePoint(point, series);
+        this.removePoint(removalPoint);
+


What's the point of this makePoint call? I left it in place just in case there's a side-effect I don't know about (is there?), but this.removePoint() is not using it.

Seems like copy/paste left overs from a long time ago. What's the point of making a point just to remove a another point?

@shefalijoshi can you take a look at this?

trusktr · 2022-01-26T19:58:48Z

src/plugins/plot/chart/MCTChartSeriesElement.js

-            insertionPoint,
-            pointsRequired
-        );
+        this.addPoint(this.makePoint(point, series), insertionPoint);


removed another unused arg that didn't do anything.

unlikelyzero

This follows the conventions that I've seen with my limited exposure to typescript. I need to defer to someone with more familiarity with the src/api to validate the rest of these changes.

unlikelyzero · 2022-01-27T14:48:03Z

@trusktr added the mocha types from our interactive demo of this functionality during testathon

trusktr · 2022-01-31T20:54:35Z

@unlikelyzero Hey, looks like tests fail because Node is too old for our build setup. My guess is that something had an in-range change that introduced syntax that causes a syntax error in older versions of Node. It'd be nice to be testing on Node 16/17. The new Node 18 LTS is coming out in April.

I think this would be a fine change, as our version of Node is what we build with, and not something that impacts consumers. If they want to build from node_modules like Vista does, they'll use whatever build tools and Node version they want.

unlikelyzero · 2022-02-01T04:23:26Z

@unlikelyzero Hey, looks like tests fail because Node is too old for our build setup. My guess is that something had an in-range change that introduced syntax that causes a syntax error in older versions of Node. It'd be nice to be testing on Node 16/17. The new Node 18 LTS is coming out in April.

I think this would be a fine change, as our version of Node is what we build with, and not something that impacts consumers. If they want to build from node_modules like Vista does, they'll use whatever build tools and Node version they want.

I don't think there's any particular project keeping us on 12. 18 will be what we need for VIPER Flight. I would imagine that we would want to continue to support 14 for open source users. Are statistics on node version information by project available from npm's regististry (i.e. openmct is built 88% of the time by node12)?

It sounds like there are at least 4 PRs stuck on node 16 support. I think we may need to bite the bullet and figure out why this is failing on node16+ #3966

…eriesCollection

unlikelyzero · 2022-02-16T22:46:12Z

@trusktr node16 has been merged in as of monday. I've rebased to pick up that change along with our circleci changes

jvigliotta · 2022-03-07T22:11:37Z

Need to follow up with @akhenry before merge as this will remove lower versions of Node support.

trusktr · 2022-03-15T20:01:02Z

14 for open source users

I normally think open source users prefer the latest, and that orgs and corporations are the ones stuck behind.

trusktr · 2022-03-15T20:08:37Z

The Node 16 build has the same issue:

  An error was thrown in afterAll
  Uncaught Error: Module build failed (from ./node_modules/istanbul-instrumenter-loader/dist/cjs.js):
  SyntaxError: Unexpected token (146:66)
      at Parser.pp$5.raise (/root/project/node_modules/babylon/lib/index.js:4454:13)
      at Parser.pp.unexpected (/root/project/node_modules/babylon/lib/index.js:1761:8)

I wonder where that problem is.

trusktr · 2022-03-15T20:09:04Z

The app works fine though, and the current log-plots stuff is branched off of this, and that works too.

trusktr · 2022-03-15T20:18:19Z

I am able to get test and test:e2e:local (with tweaks needed to both, to specify DISPLAY and CHROME_BIN) to run, no syntax error.

jvigliotta · 2022-03-21T21:07:32Z

@trusktr can you resolve the conflicts? @akhenry can you take a look at this and let us know if we're ok to lose Node 12 support?

codecov · 2022-03-23T00:54:12Z

Codecov Report

Merging #4789 (4d75aa3) into master (d30ec4c) will decrease coverage by 0.03%.
The diff coverage is 70.27%.

❗ Current head 4d75aa3 differs from pull request most recent head f25aab5. Consider uploading reports for the commit f25aab5 to get more accurate results

@@            Coverage Diff             @@
##           master    #4789      +/-   ##
==========================================
- Coverage   50.13%   50.09%   -0.04%     
==========================================
  Files         513      513              
  Lines       18848    18854       +6     
  Branches     1669     1669              
==========================================
- Hits         9449     9445       -4     
- Misses       8982     8992      +10     
  Partials      417      417

Impacted Files	Coverage Δ
src/api/time/TimeContext.js	`84.14% <ø> (ø)`
src/plugins/plot/MctPlot.vue	`51.92% <ø> (ø)`
src/plugins/plot/chart/MCTChartAlarmLineSet.js	`82.14% <ø> (ø)`
src/plugins/plot/chart/MCTChartLineLinear.js	`100.00% <ø> (ø)`
src/plugins/plot/chart/MCTChartLineStepAfter.js	`0.00% <ø> (ø)`
src/plugins/plot/chart/MCTChartPointSet.js	`100.00% <ø> (ø)`
src/plugins/plot/chart/MctChart.vue	`44.65% <0.00%> (ø)`
src/plugins/plot/configuration/LegendModel.js	`90.00% <ø> (ø)`
src/plugins/plot/draw/DrawWebGL.js	`52.47% <ø> (ø)`
src/plugins/plot/lib/eventHelpers.js	`86.20% <ø> (ø)`
... and 12 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 d30ec4c...f25aab5. Read the comment docs.

unlikelyzero · 2022-03-23T01:04:20Z

@trusktr @akhenry PR To Remove Node 12 support is here: #4963

trusktr · 2022-03-28T22:20:40Z

src/plugins/plot/MctTicks.vue

@@ -210,8 +215,8 @@ export default {
            if (this.shouldRegenerateTicks(range, forceRegeneration)) {
                let newTicks = this.getTicks();
                this.tickRange = {
-                    min: Math.min.apply(Math, newTicks),
-                    max: Math.max.apply(Math, newTicks),


Our coverage tool has recently been updated, we can freely use modern syntax now.

trusktr · 2022-03-28T22:22:11Z

src/plugins/plot/configuration/PlotConfigurationModel.js

-            legend: _.cloneDeep(_.get(options.domainObject, 'configuration.legend', {}))
+            xAxis: {},
+            yAxis: _.cloneDeep(options.domainObject.configuration?.yAxis ?? {}),
+            legend: _.cloneDeep(options.domainObject.configuration?.legend ?? {})


modern syntax ftw!

shefalijoshi

Looks good.

trusktr and others added 6 commits January 11, 2022 11:37

add some types to XAxisModel

ddf45a1

Merge branch 'master' into log-plots

4ff150c

some more type defs and small code tweaks while getting familiar with…

896571e

… plots

more type annotations and a few small tweaks

84693b0

more type annotations and small tweaks to make types show

ffaeea3

Merge branch 'master' into type-annotations

4251274

trusktr mentioned this pull request Jan 26, 2022

logarithmic axis for plots #4710

Closed

8 tasks

trusktr commented Jan 26, 2022

View reviewed changes

src/plugins/plot/configuration/XAxisModel.js Show resolved Hide resolved

trusktr commented Jan 26, 2022

View reviewed changes

trusktr requested a review from a team January 26, 2022 20:01

Merge branch 'master' into type-annotations

607acf9

trusktr mentioned this pull request Jan 26, 2022

add type annotations #4793

Closed

unlikelyzero previously approved these changes Jan 26, 2022

View reviewed changes

add mocha types

fe447a0

unlikelyzero dismissed their stale review via fe447a0 January 27, 2022 14:47

johnhill2 and others added 2 commits January 27, 2022 06:50

Add karma and jasmine, too

c9d9656

Merge branch 'master' into type-annotations

72aea12

akhenry requested review from akhenry and shefalijoshi February 7, 2022 21:55

trusktr and others added 5 commits February 15, 2022 11:31

Merge branch 'master' into type-annotations

c917914

further simplify plot canvas creation

56a2e63

further simplify plot canvas creation

429ca48

update types, avoid runtime behavior in type definition that breaks S…

f05e895

…eriesCollection

Merge branch 'master' into type-annotations

c87c9f4

Merge branch 'master' into type-annotations

817f841

trusktr and others added 3 commits March 22, 2022 10:37

Merge branch 'master' into type-annotations

5f81617

undo the changes to MctChart, improve it later

f5d4e75

Merge branch 'master' into type-annotations

08e84c9

unlikelyzero and others added 3 commits March 25, 2022 14:00

Merge branch 'master' into type-annotations

064fa80

lint fix

6a01ce0

Merge branch 'master' into type-annotations

a6d86d4

trusktr requested a review from unlikelyzero March 28, 2022 22:14

trusktr commented Mar 28, 2022

View reviewed changes

Merge branch 'master' into type-annotations

4d75aa3

trusktr enabled auto-merge (squash) March 29, 2022 19:28

trusktr requested a review from a team March 29, 2022 19:28

Reverting changes for chart elements

f25aab5

shefalijoshi approved these changes Mar 29, 2022

View reviewed changes

trusktr merged commit 651e619 into master Mar 29, 2022

shefalijoshi deleted the type-annotations branch March 29, 2022 22:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Type annotations #4789

Type annotations #4789

trusktr commented Jan 26, 2022 •

edited

Loading

trusktr Jan 26, 2022 •

edited

Loading

trusktr Jan 26, 2022

trusktr Mar 15, 2022

trusktr Jan 26, 2022

unlikelyzero left a comment

unlikelyzero commented Jan 27, 2022

trusktr commented Jan 31, 2022 •

edited

Loading

unlikelyzero commented Feb 1, 2022 •

edited

Loading

unlikelyzero commented Feb 16, 2022

jvigliotta commented Mar 7, 2022

trusktr commented Mar 15, 2022

trusktr commented Mar 15, 2022 •

edited

Loading

trusktr commented Mar 15, 2022

trusktr commented Mar 15, 2022

jvigliotta commented Mar 21, 2022

codecov bot commented Mar 23, 2022 •

edited

Loading

unlikelyzero commented Mar 23, 2022

trusktr Mar 28, 2022

trusktr Mar 28, 2022

shefalijoshi left a comment

Type annotations #4789

Type annotations #4789

Conversation

trusktr commented Jan 26, 2022 • edited Loading

Describe your changes:

All Submissions:

Author Checklist

Reviewer Checklist

trusktr Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

trusktr Jan 26, 2022

Choose a reason for hiding this comment

trusktr Mar 15, 2022

Choose a reason for hiding this comment

trusktr Jan 26, 2022

Choose a reason for hiding this comment

unlikelyzero left a comment

Choose a reason for hiding this comment

unlikelyzero commented Jan 27, 2022

trusktr commented Jan 31, 2022 • edited Loading

unlikelyzero commented Feb 1, 2022 • edited Loading

unlikelyzero commented Feb 16, 2022

jvigliotta commented Mar 7, 2022

trusktr commented Mar 15, 2022

trusktr commented Mar 15, 2022 • edited Loading

trusktr commented Mar 15, 2022

trusktr commented Mar 15, 2022

jvigliotta commented Mar 21, 2022

codecov bot commented Mar 23, 2022 • edited Loading

Codecov Report

unlikelyzero commented Mar 23, 2022

trusktr Mar 28, 2022

Choose a reason for hiding this comment

trusktr Mar 28, 2022

Choose a reason for hiding this comment

shefalijoshi left a comment

Choose a reason for hiding this comment

trusktr commented Jan 26, 2022 •

edited

Loading

trusktr Jan 26, 2022 •

edited

Loading

trusktr commented Jan 31, 2022 •

edited

Loading

unlikelyzero commented Feb 1, 2022 •

edited

Loading

trusktr commented Mar 15, 2022 •

edited

Loading

codecov bot commented Mar 23, 2022 •

edited

Loading