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

Why are dependencies linked to Github instead of NPM and semver? #14

Closed
afram opened this issue Dec 30, 2015 · 15 comments
Closed

Why are dependencies linked to Github instead of NPM and semver? #14

afram opened this issue Dec 30, 2015 · 15 comments
Milestone

Comments

@afram
Copy link
Collaborator

afram commented Dec 30, 2015

Wondering why this library and most (all?) the libraries it depends on are pulling in Github dependencies?

They currently don't support semver versioning, and this leads to unpredictable results.

For instance this change from 2 days ago in a downstream dependency broke react-masonry-component for us. We had to shrinkwrap to enforce version numbers from within git dependencies.

desandro/matches-selector@7582db4

What are the thoughts on moving dependencies over to npm?

@eiriklv
Copy link
Owner

eiriklv commented Dec 30, 2015

Hi @afram - this whole module is a bit hacky as you have experienced.

The reason for the github deps is that the ones that originally were on npm (including masonry itself) didn't work with CommonJS.

To make the whole thing work I had to clone the whole dependency tree (and some forks that had implemented CommonJS/npm support) and make the necessary changes.

Also - because of mixed use of AMD/require.js (bower) and CommonJS (npm) it didn't work with Webpack (defaults to the first supported format, which when using the UMD boilerplate is AMD). If you look at the forks and the last commit(s) you'll see what i mean.

If you want to help clear things up and get a working version which uses only npm I'd be happy to merge it / make you a collaborator with access :-)

For more info check out these issues:

@afram
Copy link
Collaborator Author

afram commented Dec 31, 2015

Yeah I see - all a bit frustrating.

I'd say there's an argument to update UMD boilerplate to put CJS first.

I'm looking into migrating this lib to npm deps.

I suspect that it should just work for Browserify users, though webpack users will need some more config.

@eiriklv
Copy link
Owner

eiriklv commented Dec 31, 2015

@afram That's the case - it just works for out of the box for browserify, but creates a lot of issues with Webpack. So back when I was working with this I couldn't find an easy solution without rewriting the UMD templates. Still waiting for webpack/webpack#883 to be handled..

@afram
Copy link
Collaborator Author

afram commented Dec 31, 2015

Another option at least for now while a better solution appears would be to reference explicit versions from github deps.

"matches-selector": "desandro/matches-selector#v1.0.3"

@eiriklv
Copy link
Owner

eiriklv commented Dec 31, 2015

@afram If you can find something that makes it work right now - I'll insta-merge it and publish a fix to npm

@afram
Copy link
Collaborator Author

afram commented Dec 31, 2015

@eiriklv I made a PR against eiriklv/outlayer which should sort this issue out. I believe No version changes are required for this to start working.

This isn't ideal, but it's probably an OK interim step to keep things working for everyone until a more robust solution is developed.

@eiriklv
Copy link
Owner

eiriklv commented Dec 31, 2015

@afram - Merged. Let me know if you need anything else.

@afram
Copy link
Collaborator Author

afram commented Dec 31, 2015

@eiriklv cheers just tested again and it works.

Will look into migrating over to NPM deps.

@eiriklv
Copy link
Owner

eiriklv commented Dec 31, 2015

@afram Awesome :-) I haven't got much time to work on this ATM, so I've made you a collaborator (both github and npm) in case you want to push fixes.

@afram
Copy link
Collaborator Author

afram commented Dec 31, 2015

oh nice - cheers :-)

desandro added a commit to desandro/masonry-docs that referenced this issue Dec 31, 2015
@desandro
Copy link

As you've noted, the issue lies with how Webpack deals with nested dependencies in issue 883. Rather than maintaining forked version of every dependency of Masonry, a simpler solution may be to use webpack.config.js. This way you can use original dependencies, as we can raise awareness around the Webpack issue. I've added Webpack instructions to Masonry - Appendix


Install Masonry and imports-loader with npm.

npm install masonry-layout imports-loader

In your config file, webpack.config.js, use the imports loader to disable define and set window for masonry-layout modules.

// npm v2
module.exports = {
  module: {
    loaders: [
      {
        test: /masonry-layout/,
        loader: 'imports?define=>false&this=>window'
      }
    ]
  }
};
// npm v3
module.exports = {
  module: {
    loaders: [
      {
        test: /masonry|fizzy\-ui\-utils|desandro\-|outlayer|get\-size|doc\-ready|eventie|eventemitter/,
        loader: 'imports?define=>false&this=>window'
      }
    ]
  }
};

(This hack is required because of an issue with how Webpack loads dependencies.)

You can then require('masonry-layout').

// main.js
var Masonry = require('masonry-layout');

var msnry = new Masonry( '.grid', {
  // options...
});

Run webpack.

webpack main.js bundle.js

jQuery plugin functionality need to be installed separately, similar to using Browserify.

@eiriklv
Copy link
Owner

eiriklv commented Dec 31, 2015

@desandro - Thanks for the alternative fix!

Neither of the solutions are any elegant IMO. You should be able to npm install without much fuss. A shame that the coincidence of AMD's if statement in the UMD spec preceding CommonJS causes these problems. Especially since Bower seems to be a dying beast. If it was the other way around this wouldn't be a problem.

@afram
Copy link
Collaborator Author

afram commented Jan 1, 2016

@eiriklv That solution might be the way to go - It's a few lines of webpack config (which sucks) but we get a lot more benefits from semver and maintained dependencies.

Thinking it'd be good to create 1x and 2x branches in case support is needed for legacy versions, and bump this to v3 - what do you think?

@eiriklv
Copy link
Owner

eiriklv commented Jan 1, 2016

@afram As long as it's documented in the README and we branch off - then I'm good. Go for it if you have the capacity :-)

@afram afram added this to the 3.0.0 milestone Jan 3, 2016
@afram
Copy link
Collaborator Author

afram commented Jan 3, 2016

fixed in 3.0.0

@afram afram closed this as completed Jan 3, 2016
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

No branches or pull requests

3 participants