Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace new Promise and .then with async/await #463

Closed
harrysarson opened this issue Oct 29, 2020 · 5 comments · Fixed by #475
Closed

Replace new Promise and .then with async/await #463

harrysarson opened this issue Oct 29, 2020 · 5 comments · Fixed by #475

Comments

@harrysarson
Copy link
Collaborator

We no longer support node 8 so there is nothing holding us back from asyncing and awaiting.


To do this

  1. Search in files for new Promise or .then.
  2. Replace it with an async function.
  3. Make a PR.

It is worth keeping the PR's small (maybe only adding a one or two async/awaits at a time) so we can review them quickly and get them merged!

@chebro
Copy link

chebro commented Oct 29, 2020

Hi, I'd like to contribute!

@harrysarson
Copy link
Collaborator Author

Hi! Go for it!

@chebro
Copy link

chebro commented Oct 29, 2020

I haven't worked with .then() blocks in a return statements a lot, it would be awesome if you could help me out here! I'm looking at this bit of code:

      return Parser.extractExposedPossiblyTests(filePath).then(
        (possiblyTests) => ({
          moduleName,
          possiblyTests,
        })
      );

I don't understand how to return a Promise.then(), using await, what I could think of is this

      possiblyTests = await Parser.extractExposedPossiblyTests(filePath)
      return (possiblyTests) => ({
          moduleName,
          possiblyTests,
        })

This will simply wait for the Promise to resolve and return the function that follows. But I'm assuming we have to return the Promise as it is and let the code that called this function that called it handle the waiting, or am I wrong?

@harrysarson
Copy link
Collaborator Author

So the trick it to make the function async so

function xxx(args) {
      ...
      return Parser.extractExposedPossiblyTests(filePath).then(
        (possiblyTests) => ({
          moduleName,
          possiblyTests,
        })
      );
}

becomes

async function xxx(args) {
      ...
      const possiblyTests = await Parser.extractExposedPossiblyTests(filePath);
      return {
          moduleName,
          possiblyTests,
      };
}

and because it is an async function it will return a promise exactly the same as before.

@chebro
Copy link

chebro commented Oct 29, 2020

So the trick it to make the function async

Yes! That's given, as the await is only valid when the function is async, I wanted to know if we are return-ing resolved promises or not. Thanks this helps, I'll make some PRs in the morning :)

@lydell lydell mentioned this issue Nov 21, 2020
harrysarson pushed a commit that referenced this issue Dec 17, 2020
This makes the watcher faster and more stable! As a bonus, this made the code much more organized.

- Concentrate all `process.exit` in elm-test.js (except one in Install.js where I found no clean solution). Besides making the code easier to follow, it allows the watcher to continue running (and recover) even if there are errors.

- Don’t crash/stop running tests if files are removed.

- Pick up new Elm files added.

- Fixes #225

  We already had code for waiting for the previous run to finish before starting the next run, but it queued up every single file change/add/remove event and ran that many runs one after the other. Now, for all watch events during a run we run _once_ after the current run for all of them. We also sleep 100 ms at the start of each run to catch more events happening close by (such as when adding/removing a whole folder of tests). (Note: We used to sleep at least 500 ms via the [awaitWriteFinish](https://github.com/paulmillr/chokidar#performance) chokidar option.)

  I’ve also tested manually that running elm-format on save doesn’t cause double test runs: #194

- Fixes #355

  We now find the closest elm.json upwards in the folder hierarchy.

- Resolves #453 (comment)

  The watcher now listens for changes to elm.json and properly recalculates things. I thought that I would have to recalculate some things only when elm.json changes for performance, but after making the globbing faster there was no need so we simply rebuild everything on each run.

- Closes #463 and closes #464

  I can’t find any more places to use async/await.

- elm.json is now decoded properly in one place rather than here and there and just being `any`. We now have 0 explicit `any` type annotations!

- elm-test.js now only contains CLI logic and subcommand dispatching. The code for running tests have been moved to RunTests.js and to FindTests.js (which used to be called Runner.js).

- Improved .flowconfig that is stricter. Closes #478

- And lots of little code improvements along the way! I only touched things that I needed to change anyway in order to not make the diff balloon too much.

I’ve done some manual testing on Windows, Mac and Linux to make sure things work like they should.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants