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

Decouple Hammer.js from Muuri #204

Closed
nathanchase opened this issue Aug 24, 2018 · 11 comments
Closed

Decouple Hammer.js from Muuri #204

nathanchase opened this issue Aug 24, 2018 · 11 comments
Labels

Comments

@nathanchase
Copy link

I'm using (and enjoying!) Muuri, but I'm not dragging anything around in my implementation.

When I install via npm (npm install muuri), I'm still seeing Hammer as a dependency when my project builds.

Is there any way to remove the require of Hammer.js so that it's not imported if I have no use for that functionality?

The only settings I require are:

layout: {
  fillGaps: true
}

Just trying to remove any unnecessary code from my project. Thank you!

image

@niklasramo
Copy link
Collaborator

Yep, hammer should probably be inside optionalDependencies instead of dependencies in package.json so you could skip installing it if need be. Will fix for 0.7.0.

@nathanchase
Copy link
Author

Wonderful. Thank you!

@niklasramo
Copy link
Collaborator

v0.7.0 is now released and hammerjs set as a peerDependency: https://github.com/haltu/muuri/blob/master/package.json#L44

@nathanchase
Copy link
Author

I'm still seeing this after updating and running npm install:
npm WARN muuri@0.7.0 requires a peer of hammerjs@^2.0.0 but none is installed. You must install peer dependencies yourself.

and then this on build:

 ERROR  Failed to compile with 1 errors                                                                
This dependency was not found:

* hammerjs in ./node_modules/muuri/dist/muuri.min.js

To install it, you can run: npm install --save hammerjs

@niklasramo
Copy link
Collaborator

Hmmm, the peerDependencies warning is expected and you can shrug it off afaik, but indeed the umd pattern is not defined as intended (there should be a trycatch around hammer require). It's automatically generated by Rollup so I'm guessing the config is off. Will look into it. Thanks for reporting 👍

@niklasramo niklasramo reopened this Sep 21, 2018
@niklasramo
Copy link
Collaborator

niklasramo commented Sep 21, 2018

The top part of muuri.js looks like this atm:

(function (global, factory) {
  typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory(require('hammerjs')) :
  typeof define === 'function' && define.amd ? define(['hammerjs'], factory) :
  (global.Muuri = factory(global.Hammer));
}(this, (function (Hammer) { 'use strict';

When it really should look like this:

(function (global, factory) {
  if (typeof exports === 'object' && typeof module !== 'undefined') {
    var Hammer;
    try { Hammer = require('hammerjs') } catch (e) {}
    module.exports = factory(Hammer);
  } else {
    global.Muuri = factory(global.Hammer);
  }
}(this, (function (Hammer) { 'use strict';

The key there is the try catch around hammerjs to make sure it's optional. I have no idea if Rollup can be configured to output this stuff. Also the AMD definition part is not necessary. Before adopting Rollup Muuri had already dropped AMD definition support.

If anyone has an idea that does not involve manually modifying the dist files after build I'm all ears. I love the development benefits that Rollup brings with it, but this is pretty annoying issue.

@nathanchase
Copy link
Author

Looks like there are a few issues around that:
rollup/rollup#1771
rollup/rollup#1039
rollup/rollup#2181

@niklasramo
Copy link
Collaborator

@nathanchase v0.7.1 is now released and added the trycatch around hammer and it should again work as it used to ;) give it a go and let me know if that's not the case.

@nathanchase
Copy link
Author

nathanchase commented Sep 30, 2018

@niklasramo Sorry to report that it still seems to be including hammer.js even in version 0.7.1:

image

@niklasramo
Copy link
Collaborator

It does that with just npm install? Well, we could always just remove hammer totally from package.json if nothing else helps.

Although, there's not gonna be hammer anymore in the next version of muuri... custom implementation 95% done.

@niklasramo
Copy link
Collaborator

Hammer is removed fully from the upcoming version so closing this.

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

No branches or pull requests

2 participants