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

Simpler and better cordova/util.getPlatformApiFunction #767

Merged
merged 2 commits into from
Apr 17, 2019

Conversation

raphinesse
Copy link
Contributor

@raphinesse raphinesse commented Apr 13, 2019

Motivation and Context

The logic in cordova/util.getPlatformApiFunction was a tad to complex and had a few problems because of that.

During testing I frequently had the situation that an error occurred during loading of a platform API module, mostly because of a missing dependency. This function caught these errors and insted threw an error claiming 'Your ' + platform + ' platform does not have Api.js' which was plainly wrong and confusing.

On top of that came a dead code branch and IMHO unnecessary checks. Enough reason for a rewrite

AFAICT this method is not publicly exposed, so this should be a minor change at most.

Description

  • The happy path is unchanged as are the situations in which errors are thrown
  • The second argument is now unused and could be removed at the call sites in a following or this PR
  • The other notable changes all pertain to logged messages:
    • No more warning that file should be called Api.js
    • No more special error message if platform is a custom one (!platforms[platform])
    • Improved error message if module doesn't implement the expected interface

Testing

The existing tests pass.

@codecov-io
Copy link

codecov-io commented Apr 13, 2019

Codecov Report

Merging #767 into master will increase coverage by 0.14%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #767      +/-   ##
==========================================
+ Coverage   85.71%   85.86%   +0.14%     
==========================================
  Files          50       50              
  Lines        2598     2589       -9     
==========================================
- Hits         2227     2223       -4     
+ Misses        371      366       -5
Impacted Files Coverage Δ
src/cordova/util.js 93.02% <50%> (+2.41%) ⬆️

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 07a0dd8...66c41f1. Read the comment docs.

Copy link
Member

@dpogue dpogue left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@erisu erisu left a comment

Choose a reason for hiding this comment

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

LGTM, only made small changes in my opinion but not actually required.

src/cordova/util.js Outdated Show resolved Hide resolved
src/cordova/util.js Outdated Show resolved Hide resolved
src/cordova/util.js Outdated Show resolved Hide resolved
spec/cordova/platforms/platforms.spec.js Outdated Show resolved Hide resolved
spec/cordova/util.spec.js Outdated Show resolved Hide resolved
Co-Authored-By: raphinesse <raphinesse@gmail.com>
@dpogue dpogue merged commit 3be2acb into apache:master Apr 17, 2019
@raphinesse raphinesse deleted the simpler-get-api branch April 17, 2019 00:09
This pull request was closed.
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.

4 participants