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

Rewrite async code and Remove bluebird #782

Merged
merged 11 commits into from
Nov 6, 2022

Conversation

MikuroXina
Copy link
Collaborator

@MikuroXina MikuroXina commented Oct 27, 2022

Resolves #781. This introduces a breaking change on readAsync function in bms package.

Changelog

Improved the code that handles asynchronous processing to more closely follow latest coding practices and removed a 3rd party dependency.

For developers using Bemuse’s published npm libraries — there is a breaking change in the bms library where the readAsync function no longer accepts a callback and now returns a Promise instead.

@dtinth
Copy link
Member

dtinth commented Oct 27, 2022

Woah, many thanks! That seems like a lot of work.

Regarding the breaking change, please run rush change to generate a change file that will help the publish scripts bump the version correctly.

@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2022

Codecov Report

Base: 81.05% // Head: 80.98% // Decreases project coverage by -0.07% ⚠️

Coverage data is based on head (7302ac9) compared to base (a6194df).
Patch coverage: 37.37% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #782      +/-   ##
==========================================
- Coverage   81.05%   80.98%   -0.08%     
==========================================
  Files         178      177       -1     
  Lines        5290     5270      -20     
  Branches      242      240       -2     
==========================================
- Hits         4288     4268      -20     
- Misses        931      933       +2     
+ Partials       71       69       -2     
Impacted Files Coverage Δ
bemuse/src/bootstrap/index.js 100.00% <ø> (ø)
...emuse/src/custom-song-loader/song-loader.worker.js 100.00% <ø> (ø)
bemuse/src/online/index.js 0.00% <0.00%> (ø)
bemuse/src/online/index.spec.js 4.11% <0.00%> (+0.04%) ⬆️
bemuse/src/utils/download.js 96.15% <ø> (ø)
packages/bms/features/support/setup.js 100.00% <ø> (ø)
bemuse/src/progress/utils.ts 71.79% <20.00%> (-0.43%) ⬇️
packages/bms/src/reader/index.ts 82.35% <75.00%> (+13.12%) ⬆️
bemuse/src/custom-song-loader/index.ts 63.63% <88.88%> (ø)
bemuse/src/custom-song-loader/index.spec.js 100.00% <100.00%> (ø)
... and 14 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@dtinth dtinth left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in reviewing this PR. Since there are many changes and we don’t have enough automated tests, I had to find a focus time to review this in more depth.

There are few concerns with the promise pool API (w.r.t. error handling) and one instance where the .done() method is removed from a non-Promise.

Thanks again for contributing!

bemuse/src/game/input/touch-plugin.js Outdated Show resolved Hide resolved
bemuse/src/game/loaders/samples-loader.js Outdated Show resolved Hide resolved
bemuse/src/game/ui/LoadingScene.jsx Outdated Show resolved Hide resolved
bemuse/tasks/build.js Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Nov 6, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@dtinth dtinth left a comment

Choose a reason for hiding this comment

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

Everything looks great now! Thank you so much 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove bluebird
3 participants