Moving to async/await: lib/Runner.js

[chebro]

This PR attempts to close #463

[chebro]

Just checked Travis log, I found this:

mod.possiblyTests.map is not a function

Am I doing something wrong?

[harrysarson]

@lydell this is a potential downside of the better error handling we have rolled out. We do no longer get stack traces for errors like these.

@sravanth-chebrolu it looks like somewhere you are returning an object containing a Promise rather than a Promise to an object. Try running npm check I think flow should tell you where you are returning a slightly wrong thing.

[lydell]

@harrysarson Flow should catch type errors.

EDIT: Which it does.

$ npx flow
Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ lib/Runner.js:11:10

Cannot return Promise.all(...) because Promise [1] is incompatible with Array [2] in property possiblyTests of array
element of type argument R [3]. [incompatible-return]

     lib/Runner.js
       8│   sourceDirs /*: Array<string> */,
       9│   isPackageProject /*: boolean */
 [2]  10│ ) /*: Promise<Array<{ moduleName: string, possiblyTests: Array<string> }>> */ {
      11│   return Promise.all(
      12│     testFilePaths.map((filePath) => {
      13│       const matchingSourceDirs = sourceDirs.filter((dir) =>
      14│         filePath.startsWith(`${dir}${path.sep}`)
      15│       );
      16│
      17│       // Tests must be in tests/ or in source-directories – otherwise they won’t
      18│       // compile. Elm won’t be able to find imports.
      19│       switch (matchingSourceDirs.length) {
      20│         case 0:
      21│           return Promise.reject(
      22│             Error(missingSourceDirectoryError(filePath, isPackageProject))
      23│           );
      24│
      25│         case 1:
        :
      46│       if (!moduleNameParts.every(Parser.isUpperName)) {
      47│         return Promise.reject(
      48│           new Error(
      49│             badModuleNameError(filePath, matchingSourceDirs[0], moduleName)
      50│           )
      51│         );
      52│       }
      53│
      54│       const possiblyTests = Parser.extractExposedPossiblyTests(filePath)
      55│       return {
      56│           moduleName,
      57│           possiblyTests,
      58│         }
      59│     })
      60│   );
      61│ }
      62│
      63│ function missingSourceDirectoryError(filePath, isPackageProject) {

     /private/tmp/flow/flowlib_b3e8825/core.js
 [3] 682│ declare class Promise<+R> {

     lib/Parser.js
 [1]  32│ ) /*: Promise<Array<string>> */ {



Found 1 error

@sravanth-chebrolu I recommend running Flow and ESLint and Pretter in your editor!

[chebro]

Yeah I found the error, it was a stupid mistake, I forgot the await keyword. Sorry for wasting your time :(

[chebro]

Weird.. tests pass for Nodejs v15, but fail for the rest

[harrysarson]

CI failures are because you need to put more of the function calls within the try catch

[chebro]

Thanks I see the mistakes, I've learnt so much about promises from just this PR..

[chebro]

Should we merge #465? I don't understand why the checks are still failing

[lydell]

@chebro The checks are failing because ESLint fails to parse catch without (foo) after it. This can be solved in three ways:

1. Use catch (_error) instead.
2. Bump ecmaVersion in .eslintrc.js from 2018 to 2019. I’m not sure if this is safe in general for Node.js 10+ support.
3. Merge Improve elm-test.js #465 first which does not this problem – when I converted to async/await there I did not end up ignoring any errors.

[chebro]

1. Use catch (_error) instead.

Will do this 👍🏽

3. Merge Improve elm-test.js #465 first which does not this problem – when I converted to async/await there I did not end up ignoring any errors.

Should I accept all incoming changes?

[lydell]

Should I accept all incoming changes?

Yes, that’s the plan once #465 is merged into master. (Note: There are one or two .then left in my version where I considered that more readable.) We’ll see which PR is merged first and who gets to solve the conflicts :)

[chebro]

This is my first merge (with a conflict) on github, hope I did it right 🤞

[lydell]

@chebro I didn’t mean that you should merge #465 into this branch yet. But maybe things will sort themselves out if we merge #465 into master and then squash merge this one. 🤔

[chebro]

I'll just wait until your branch is merged then. I can still hard reset if it comes to it..

[harrysarson]

CI is green 👍 marking as blocked as I cannot review due to all the changes pulled in from the other PR?

[chebro]

due to all the changes pulled in from the other PR?

Yes, I misunderstood what @lydell said, my bad.

[harrysarson]

I am happy to merge this before #465 if it gets untangled. Equally I am very happy to wait :)

[harrysarson]

You are free to go for it @chebro!

[chebro]

You are free to go for it @chebro!

Aye! I'm a little busy this week, will start as soon as I can.

[harrysarson]

No worries! Take your time (amongst other things of ci is broken so we can't merge until that is fixed anyway).

[chebro]

Hmm, there seem to be some conflicts now, should I pull the master branch into this?

[harrysarson]

Yeah! Don't worry about the commit history looking nice because I will squash when I merge

[lydell]

Also note that this PR is not needed anymore if #475 is merged. Sorry!

[chebro]

Also note that this PR is not needed anymore if #475 is merged. Sorry!

Alright, we can close this then. I'm glad that I got to take away something I previously didn't know about promises from this PR. Good luck :)

[harrysarson]

Thanks @chebro, as they say "code is the easy part". Most the valuable work in open source is exploring the design space and working out what code you actually need, that is to say: PRs that do not get merged are no less valuable to a project as those that do. Till next time ...