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

Allow Hammer.js to exist as an npm-installed package #787

Merged
merged 2 commits into from
Mar 1, 2018
Merged

Conversation

brianhelba
Copy link
Contributor

When resolving require('hammerjs'), this will look for npm-installed hammerjs packages to link to, in addition to a global Hammer variable.

Since hammerjs is an npm-optional dependency of GeoJS, if Webpack rebundles GeoJS in a downstream project, it will be smart enough to elide any require('hammerjs') statements in the built GeoJS. Without this change, downstream Webpack is not able to elide require('Hammer') usages (since the module name doesn't match the optional dependency name).

Fixes #786

When resolving "require('hammerjs')", this will look for npm-installed
"hammerjs" packages to link to, in addition to a global "Hammer" variable.

Since "hammerjs" is an npm-optional dependency of GeoJS, if Webpack
rebundles GeoJS in a downstream project, it will be smart enough to elide
any "require('hammerjs')" statements in the built GeoJS. Without this
change, downstream Webpack is not able to elide "require('Hammer')" usages
(since the module name doesn't match the optional dependency name).
@brianhelba
Copy link
Contributor Author

@OpenGeoscience/developers If / when this is merged, can we get another patch release?

Copy link
Contributor

@manthey manthey left a comment

Choose a reason for hiding this comment

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

I haven't been able to reproduce the warning you saw, but this looks fine.

@manthey manthey merged commit 4617718 into master Mar 1, 2018
@manthey manthey deleted the hammerjs branch March 1, 2018 16:24
@manthey
Copy link
Contributor

manthey commented Mar 1, 2018

@brianhelba I see in the webpack build, that the error thrown by webpack when hammerjs isn't found reports Cannot find module "undefined". Although this should never be reached (since it is optional), is there a way to have the proper name here?

@brianhelba
Copy link
Contributor Author

@manthey I noticed this already, but didn't really care, since hammerjs is optional and so Webpack should never throw an error if its lookup fails (per the fix from webpack/webpack#339 ). However, other types of downstream usages could be helped by a better error message, so I've fixed this here: #789

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.

2 participants