[Flow] src/utils/function.js #5904

atwalg2 · 2018-04-10T16:21:35Z

Fixes Issue: #5120

SymbolDeclarations? or should I just put Object?

jasonLaster · 2018-04-10T16:23:17Z

src/utils/function.js

+  line: number,
+  source: any,
+  symbols: SymbolDeclarations
+): string | null {


looks good.

I think we need to import SymbolDeclarations and perhaps replace any with any

replace any with any?

oops, i meant Source

atwalg2 · 2018-04-12T06:41:07Z

ok so issue is here:

export function findFunctionText(
  line: number,
  source: any,
  symbols: SymbolDeclarations
): string | null {
  const func = findClosestFunction(symbols, {
    line,
    column: Infinity
  });
  if (!func) {
    return null;
  }

the second arguemnt of findClosestFunction is a location type.. i.e

export type Location = {
  sourceId: SourceId,
  line: number,
  column: ?number,
  sourceUrl?: string
};

simple solution is to make sourceId an optional property. Thoughts?

jasonLaster · 2018-04-12T18:03:38Z

src/types.js

@@ -54,7 +54,7 @@ export type ActorId = string;
 * @static
 */
 export type Location = {
-  sourceId: SourceId,
+  sourceId?: SourceId,


sorry, this shouldnt be optional, the param for the reducer should be

This is still a problem if I change this back and edit the reducer param.
In ast.js, location is declared multiple times and if sourceId is a required param then it should be included.. seperated... or made optional

jasonLaster · 2018-04-13T12:10:41Z

src/utils/function.js

+    });
+  } else {
+    return null;
+  }


small suggestion here. I think i'd prefer putting the guard statement in findClosestFunction. We prefer safer calls than checks before the calls

diff --git a/src/utils/ast.js b/src/utils/ast.js index 56c30f3..6464345 100644 --- a/src/utils/ast.js +++ b/src/utils/ast.js @@ -104,17 +104,25 @@ function findClosestofSymbol(declarations: any[], location: Location) { } export function findClosestFunction( - symbols: SymbolDeclarations, + symbols: ?SymbolDeclarations, location: Location ) { + if (!symbols) { + return null; + } + const { functions } = symbols; return findClosestofSymbol(functions, location); } export function findClosestClass( - symbols: SymbolDeclarations, + symbols: ?SymbolDeclarations, location: Location ) { + if (!symbols) { + return null; + } + const { classes } = symbols; return findClosestofSymbol(classes, location); }

jasonLaster · 2018-04-13T12:10:41Z

src/reducers/ast.js

+export function getSymbols(
+  state: OuterState,
+  source: Source
+): ?SymbolDeclarations {


It's important that this is Symbols and not SymbolDeclarations look at the definition above

hey @jasonLaster were you referring to only this case or the other ones as well. I may have read this wrong initially.. I just realized you may have meant only this case and not the others .. oops. Please clarify this for me I can make a quick fix

AnshulMalik

Nice! And a rebase always helps.

AnshulMalik · 2018-04-19T08:21:41Z

src/utils/function.js

-  const middle = lines.slice(start.line, end.line - 1);
-  const functionText = [firstLine, ...middle, lastLine].join("\n");
-  const indentedFunctionText = correctIndentation(functionText);
+  const lines: string[] = source.text.split("\n");


Do we need these types?

No we don't. I just put them in their to try and improve the file coverage, which most likely wouldn't affect it.

AnshulMalik · 2018-04-19T08:44:36Z

src/utils/ast.js

+  if (
+    !(symbols.memberExpressions && symbols.identifiers && symbols.identifiers)
+  ) {
+    return null;


This means both memberExpressions and identifiers should be present for this to return results. Is this what we want?

jasonLaster · 2018-04-19T12:39:15Z

src/utils/ast.js

+  if (
+    !(symbols.memberExpressions && symbols.identifiers && symbols.identifiers)
+  ) {
+    return null;


!(symbols.memberExpressions && symbols.identifiers && symbols.identifiers)

could we just do !symbols

well no because as per type def for Symbols it can be either SymbolDeclaration | {| loading: true |}. So we have to account for the other case. Really it seems like we are only dealing with SymbolDeclarations here. What do you think?

jasonLaster · 2018-04-19T12:39:15Z

src/utils/function.js

+  const lastLine: string = lines[end.line - 1].slice(0, end.column);
+  const middle: string[] = lines.slice(start.line, end.line - 1);
+  const functionText: string = [firstLine, ...middle, lastLine].join("\n");
+  const indentedFunctionText: string = correctIndentation(functionText);


do we need these for it to pass? they should be inferred...

oh ok I can get rid of these. They are def unnecessary.

jasonLaster · 2018-04-19T16:46:13Z

src/utils/ast.js

+  if (
+    !(symbols.memberExpressions && symbols.identifiers && symbols.identifiers)
+  ) {
+    return null;


so symbols.identifiers && symbols.identifiers is unneccessary right?

could we do !symbols.loading && symbols

atwalg2 · 2018-04-20T15:45:53Z

src/utils/ast.js

-  const { line, column } = tokenPos;
+  if (symbols.loading) {
+    return null;
+  }



@jasonLaster hey Jason so I think having if(symbols.loading) return null make sense in this case. What do you think?

yes. that definitely works

This reverts commit 6b2ac3a.

jasonLaster reviewed Apr 10, 2018

View reviewed changes

jasonLaster added the 📚 pr-wip label Apr 12, 2018

jasonLaster reviewed Apr 12, 2018

View reviewed changes

jasonLaster reviewed Apr 13, 2018

View reviewed changes

jasonLaster changed the title ~~[Flow] src/utils/function.js - add type defs for flow~~ [Flow] src/utils/function.js Apr 13, 2018

AnshulMalik reviewed Apr 19, 2018

View reviewed changes

jasonLaster reviewed Apr 19, 2018

View reviewed changes

atwalg2 commented Apr 20, 2018

View reviewed changes

jasonLaster force-pushed the flow-function.js branch from 94fd973 to a0d51ed Compare April 24, 2018 16:27

add & improve flow coverage

1ec3aa3

jasonLaster force-pushed the flow-function.js branch from a0d51ed to 1ec3aa3 Compare April 24, 2018 16:29

jasonLaster merged commit 6b2ac3a into firefox-devtools:master Apr 24, 2018

jasonLaster added a commit to jasonLaster/debugger.html that referenced this pull request Apr 25, 2018

Revert "[Flow] src/utils/function.js (firefox-devtools#5904)"

a6fcd1b

This reverts commit 6b2ac3a.

jasonLaster pushed a commit that referenced this pull request Apr 25, 2018

[Flow] src/utils/function.js (#5904)

76fde84

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Flow] src/utils/function.js #5904

[Flow] src/utils/function.js #5904

atwalg2 commented Apr 10, 2018

jasonLaster Apr 10, 2018

atwalg2 Apr 12, 2018

jasonLaster Apr 12, 2018

atwalg2 commented Apr 12, 2018 •

edited

Loading

jasonLaster Apr 12, 2018

atwalg2 Apr 13, 2018

atwalg2 Apr 13, 2018

jasonLaster Apr 13, 2018

jasonLaster Apr 13, 2018

atwalg2 Apr 19, 2018

AnshulMalik left a comment

AnshulMalik Apr 19, 2018

atwalg2 Apr 19, 2018

AnshulMalik Apr 19, 2018

jasonLaster Apr 19, 2018

atwalg2 Apr 19, 2018

jasonLaster Apr 19, 2018

atwalg2 Apr 19, 2018

jasonLaster Apr 19, 2018

atwalg2 Apr 20, 2018

jasonLaster Apr 20, 2018 •

edited

Loading

[Flow] src/utils/function.js #5904

[Flow] src/utils/function.js #5904

Conversation

atwalg2 commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atwalg2 commented Apr 12, 2018 • 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

Choose a reason for hiding this comment

AnshulMalik 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

jasonLaster Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

atwalg2 commented Apr 12, 2018 •

edited

Loading

jasonLaster Apr 20, 2018 •

edited

Loading