Skip to content
New issue

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

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

Already on GitHub? # to your account

Make report async #23

Merged
merged 2 commits into from
Oct 16, 2020
Merged

Make report async #23

merged 2 commits into from
Oct 16, 2020

Conversation

Arkham
Copy link
Contributor

@Arkham Arkham commented Oct 15, 2020

Hello there! I've bumped into the following issue.

With this setup:

$ node --version
v13.8.0
$ npm --version
6.13.6
$ npx elm-review --version
2.3.0

Running the following commands:

$ git clone git@github.com:jfmengels/elm-review-unused.git
$ cd elm-review-unused
$ npm install
$ npm test

Led to this error:

An error occurred while trying to check whether the preview configuration compiles.
Error: Command failed: npx elm-review --config /Users/arkham/code/elm-review-unused-test/preview --report=json
    at checkExecSyncError (child_process.js:611:11)
    at execSync (child_process.js:647:15)
    at checkThatExampleCompiles (/Users/arkham/code/elm-review-unused-test/elm-review-package-tests/check-previews-compile.js:16:5)
    at Array.forEach (<anonymous>)
    at Object.<anonymous> (/Users/arkham/code/elm-review-unused-test/elm-review-package-tests/check-previews-compile.js:12:29)
    at Module._compile (internal/modules/cjs/loader.js:1151:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1171:10)
    at Module.load (internal/modules/cjs/loader.js:1000:32)
    at Function.Module._load (internal/modules/cjs/loader.js:899:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)

Which was caused by the output of elm-review being cut at 8192 characters.

Doing some digging this seems to be a common issue in NodeJS when doing something like:

console.log(veryLargeString);
process.exit();

Check out these links for more info:

So in this PR I changed the report function to be async and replaced console.log with process.stdout.write, which allows to be passed a callback when the write has been finished (https://nodejs.org/api/stream.html#stream_writable_write_chunk_encoding_callback). I also promisified it so we don't have to pass that callback from the normal code.

With this fix in place, running npm test in the above setup succeeds.

I have no idea about NodeJS development in general, so please let me know if this looks good to you.

@Arkham
Copy link
Contributor Author

Arkham commented Oct 15, 2020

Test failed again due to github auth token missing, any way to fix that on travis?

Copy link
Owner

@jfmengels jfmengels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this!

Test failed again due to github auth token missing, any way to fix that on travis?

That's weird, because the token is there 🤔
I'll look into switching to GitHub actions, since I'm moving everything there anyway for other repositories.

The tests pass for me locally anyway, so the CI is not a blocker.

Apart from that, there is one bug that was introduced but easily fixed.

@jfmengels
Copy link
Owner

I've replaced Travis by GitHub actions on master, feel free to rebase (and I hope it will work)

@Arkham Arkham force-pushed the make-report-async branch from 5469c4a to f6d9a1a Compare October 15, 2020 21:49
@Arkham Arkham force-pushed the make-report-async branch from f6d9a1a to 0c4c68f Compare October 15, 2020 21:56
@Arkham
Copy link
Contributor Author

Arkham commented Oct 15, 2020

Bug fixed and CI went green!

Copy link
Owner

@jfmengels jfmengels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks a lot @Arkham!

@jfmengels jfmengels merged commit 301a561 into jfmengels:master Oct 16, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants