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

implement async API #42

Merged
merged 15 commits into from
Mar 2, 2019
Merged

implement async API #42

merged 15 commits into from
Mar 2, 2019

Conversation

axe312ger
Copy link
Owner

@axe312ger axe312ger commented Jul 11, 2018

This is hopefully the last huge rewrite on the road to v1 #35 #30

  • changes api to async code using babel
  • removes shouldThrow since it was just clutter
  • changes to last svgo which is async only
  • extends e2e tests to cover help and missing parameter output
  • fixes bug when running multiple primitive at once (all instances shared a temporary file)

Will fix #5 when done

@axe312ger axe312ger requested a review from efegurkan July 13, 2018 16:38
const PRIMITIVE_TEMP_FILE = os.tmpdir() + '/primitive_tempfile.svg'
const PRIMITIVE_TEMP_FILE = path.resolve(
os.tmpdir(),
`primitive-tempfile-${new Date().getTime()}.svg`
Copy link

Choose a reason for hiding this comment

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

I recommend using https://www.npmjs.com/package/tempfile here to avoid possible tempfile clashes when many files are worked on in parallel

Copy link
Owner Author

Choose a reason for hiding this comment

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

Cool idea!! Maybe even https://github.com/sindresorhus/tempy since it is recommended in the tempfile README.md :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Done, see #45

@axe312ger axe312ger mentioned this pull request Jul 17, 2018
7 tasks
@maxnordlund
Copy link
Contributor

The tests failures looks odd, the first about missing localStorage and the second about syntax errors in golang's draw module. Maybe the latter has resolved itself since the tests were run, but the former I'm not sure about.

Either way it would be really sweet if this got merged 🙌

@codecov-io
Copy link

codecov-io commented Mar 2, 2019

Codecov Report

❗ No coverage uploaded for pull request base (next@3fdaaa1). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@         Coverage Diff          @@
##             next   #42   +/-   ##
====================================
  Coverage        ?    0%           
====================================
  Files           ?     5           
  Lines           ?   100           
  Branches        ?    11           
====================================
  Hits            ?     0           
  Misses          ?    89           
  Partials        ?    11
Impacted Files Coverage Δ
src/utils/helpers.js 0% <ø> (ø)
src/cli.js 0% <0%> (ø)
src/utils/svg.js 0% <0%> (ø)
src/index.js 0% <0%> (ø)
src/utils/primitive.js 0% <0%> (ø)

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 3fdaaa1...eeee03a. Read the comment docs.

@axe312ger
Copy link
Owner Author

@hafizalfaza your commit failed on CircleCI

(function (exports, require, module, __filename, __dirname) { import { execSync } from 'child-process'

We need to keep require in the bundle-vendor.js since it is executed by node directly without babel involved.

@axe312ger
Copy link
Owner Author

Alright, looks good for now. lets continue in another PR/branch :)

@axe312ger axe312ger merged commit eb50f5d into next Mar 2, 2019
@axe312ger axe312ger deleted the refactor/async branch March 2, 2019 07:43
@axe312ger axe312ger added this to the 1.0.0 milestone Jun 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants