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

Better checks for external modules. #696

Merged
merged 5 commits into from
May 11, 2017
Merged

Better checks for external modules. #696

merged 5 commits into from
May 11, 2017

Conversation

manthey
Copy link
Contributor

@manthey manthey commented May 2, 2017

D3 and HammerJS are intended to be optional modules. Consumers of geojs (at least those that don't use webpack) improperly report that these modules are always available.

D3 and HammerJS are intended to be optional modules.  Consumers of geojs (at least those that don't use webpack) improperly report that these modules are always available.
@codecov-io
Copy link

codecov-io commented May 2, 2017

Codecov Report

Merging #696 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #696      +/-   ##
==========================================
+ Coverage   95.26%   95.26%   +<.01%     
==========================================
  Files          84       84              
  Lines        8990     8994       +4     
==========================================
+ Hits         8564     8568       +4     
  Misses        426      426
Impacted Files Coverage Δ
src/d3/d3Renderer.js 93.35% <100%> (+0.07%) ⬆️
src/mapInteractor.js 95.83% <100%> (ø) ⬆️

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 aff21e4...b122f12. Read the comment docs.

@jbeezley
Copy link
Contributor

jbeezley commented May 2, 2017

We might also want to change the public symbol for hammerjs in the webpack config (https://github.com/OpenGeoscience/geojs/blob/master/webpack.config.js#L46). If you include hammerjs as a global, it is exported to window.Hammer.

@manthey
Copy link
Contributor Author

manthey commented May 2, 2017

@jbeezley Is the change I made sufficient? Is there some sensible way to add a test for gracefully not using Hammer if it isn't available?

@jbeezley
Copy link
Contributor

jbeezley commented May 2, 2017

The change looks good. I don't know of any good ways of testing it.

Copy link
Member

@aashish24 aashish24 left a comment

Choose a reason for hiding this comment

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

LRTM as @jbeezley also approved it (thanks)

@manthey manthey merged commit c0c5a7a into master May 11, 2017
@manthey manthey deleted the better-webpack-check branch May 11, 2017 12:17
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.

4 participants