metafizzy / outlayer

:construction_worker: the brains & guts of a layout library
163 stars 63 forks source link

Wiredep bug - item.js missing in bower.json main #33

Open Lucas-C opened 9 years ago

Lucas-C commented 9 years ago

Hello,

It appears that commit 914f1f2 introduced a bug: https://github.com/metafizzy/outlayer/commit/914f1f2fa9874935dc7723a5a1171e0f78a689e7#diff-a63318b17de3bf77b5cd6eb71e7aae44R46

How can window.Outlayer.Item be defined before assigning the result of the factory call to window.Outlayer ?

My Karma tests fail on this when launching PhantomJS, with the following error:

Running "karma:dev" (karma) task
INFO [karma]: Karma v0.12.31 server started at http://localhost:9876/
INFO [launcher]: Starting browser PhantomJS
INFO [PhantomJS 1.9.8 (Windows 8)]: Connected on socket _xqtqhxXOy8qR62SOmNJ with id 26908454
PhantomJS 1.9.8 (Windows 8) ERROR
  TypeError: 'undefined' is not an object (evaluating 'window.Outlayer.Item')
  at src/main/bower_components/outlayer/outlayer.js:42

Or did I got the root cause wrong ?

Lucas-C commented 9 years ago

Actually this already happened in v1.3.0

Looks like @dimerman had a similar issue back in 2013 : https://github.com/metafizzy/outlayer/commit/a0d5481f1ea0bac982f2d22e90118ac40fd3e071

Seems like I "need to have item.js before outlayer.js".

But outlayer is just a dependency of packery for me : shouldn't item.js be automagically included as a dependency by bower ? I does not appear when I run bower list.

desandro commented 9 years ago

bower's main has been poorly defined in the past. While previous versions of Outlayer did include item.js in main, Outlayer v1.3 has adopted a new specification for main. See bower/bower.json-spec#43.

Unfortunately, this may break build tools. Thanks for reporting this issue.

EHLOVader commented 9 years ago

I was affected by this as I use wiredep for loading the bower dependencies into my web projects. I can understand the single file entry point philosophy in bower only if the file actually serves as a single entry point.

Either a single concatenated distribution file or a single file which requires all others in some way.

Until this is resolved I have added the below override to my growing list of overrides to patch scenarios like this one.

Hopefully this helps others.

  "overrides": {
    "outlayer": {
      "main": [
        "item.js",
        "outlayer.js"
      ]
    }
  }
Lucas-C commented 9 years ago

Thanks @EHLOVader ! That did the trick for me

jesusjackson commented 9 years ago

Sorry to bother @EHLOVader but i'm a noob. Does the override snipet you showed goes on the bower.json after the dependencies or somewhere different?

EHLOVader commented 9 years ago

@jesusjackson you've got it!

You just add an overrides property to your bower.json object. It can go anywhere afaik. I think the property is "unofficial" but I can't see it going anywhere any time soon. So many people depend on it for things like this. Here are details from Roots.io, was the best doc I found on the topic.

https://roots.io/using-bootstrap-with-bower-how-to-override-bower-packages/

jesusjackson commented 9 years ago

Thanks for the quick answer @EHLOVader, but apparently grunt is ignoring the overrides

"overrides": {
    "isotope": {
      "main": ["./dist/isotope.pkgd.min.js"]
    },
    "outlayer": {
      "main": [
        "item.js",
        "outlayer.js"
      ]
    }
  }

Is generating these two lines instead of the ones i specified on the override:

    <script src="bower_components/outlayer/outlayer.js"></script>
    <script src="bower_components/masonry/masonry.js"></script>
    <script src="bower_components/isotope/js/isotope.js"></script>
EHLOVader commented 9 years ago

@jesusjackson That is certainly odd. Perhaps not using a ./ relative location for your main file.

This override that I have here is only a fraction of the full override I have started using myself. Since the definition of the main property was changed and only single entry points were recommended I have had to override many projects which weren't built for a single entry point, like this one.

I think a pretty complete override that worked for me is here https://github.com/metafizzy/isotope/issues/879#issuecomment-94335043

I had to include the dependencies on that one for jquery-bridget support.

If that doesn't work what are you using for grunt to handle the dependencies? I use gulp myself, but I see gulp-bower-install mentioned in a few comment threads that suggest switching to gulp-wiredep or just newer versions.

Good luck.

jesusjackson commented 9 years ago

No use @EHLOVader, apparently bower hates me. I use wiredep, but even with your full override. I'll put my bower.json here. Thanks for all the help.

"overrides": {
    "isotope": {
      "main": [
        "js/item.js",
        "js/layout-mode.js",
        "js/isotope.js",
        "js/layout-modes/vertical.js",
        "js/layout-modes/fit-rows.js",
        "js/layout-modes/masonry.js"
      ],
      "dependencies": {
        "get-size": ">=1.1.8 <1.3",
        "matches-selector": ">=1 <2",
        "outlayer": "1.3.x",
        "masonry": "3.2.x",
        "jquery": ">=1.4.3 <2",
        "jquery-bridget": "1.1.x"
      }
    },
    "outlayer": {
      "main": [
        "item.js",
        "outlayer.js"
      ]
    }
  }
danimalweb commented 8 years ago

Thanks @jesusjackson this helped me out.

OmgImAlexis commented 8 years ago

For anyone using grunt-bower-concat try something like this.

module.exports = function(grunt) {
    grunt.initConfig({
        bower_concat: {
            all: {
                dest: './_bower.js',
                cssDest: './_bower.css',
                exclude: [
                ],
                dependencies: {
                },
                mainFiles: {
                    'isotope': [
                        "dist/isotope.pkgd.min.js"
                    ],
                    "outlayer": [
                        "item.js",
                        "outlayer.js"
                    ]
                },
                bowerOptions: {
                    relative: false
                }
            }
        }
    });

    grunt.loadNpmTasks('grunt-bower-concat');

    grunt.registerTask('default', ['bower_concat']);
};
PhilippSoehnlein commented 8 years ago

@OmgImAlexis You shouldn't do it this way, you will end up including Outlayer twice in your generated file, because isotope.pkgd.min.js already includes it.

Here's my version:

module.exports = function(grunt) {
    grunt.initConfig({
        bower_concat: {
            yourBuildTarget: {
                mainFiles: {
                    // outlayer (and isotope) have a main-definition that conflicts with non-module loader setups.
                    // See https://github.com/metafizzy/outlayer/issues/33
                    'outlayer':   ['item.js', 'outlayer.js'],
                    'isotope':    [
                        'js/item.js',
                        'js/layout-mode.js',
                        'js/layout-modes/fit-rows.js', 'js/layout-modes/masonry.js', 'js/layout-modes/vertical.js',
                        'js/isotope.js',
                    ],
                }
            }
        }
    });
    // …
}
lucasengel commented 8 years ago

Isn't there an official resolution for this yet?

EHLOVader commented 8 years ago

It basically is resolved.

From the comment above: https://github.com/metafizzy/outlayer/issues/33#issuecomment-93809453

bower's main has been poorly defined in the past. While previous versions of Outlayer did include item.js in main, Outlayer v1.3 has adopted a new specification for main. See bower/spec#43.

Unfortunately, this may break build tools. Thanks for reporting this issue.

There is still some discussion over here https://github.com/bower/spec/issues/47 on ways to improve this beyond just using overrides, but overrides or using the pkgd dist file is the official solution afaik.

lucasengel commented 8 years ago

Thanks @EHLOVader!

For anyone using Grunt, here's my version on fixing Masonry dependencies:

wiredep: {
  options: {
    "overrides": {
      "ev-emitter": {
        "dependencies": {
          "jquery-bridget": ">=2.0.0"
        }
      },
      "outlayer": {
        "main": [
          "item.js",
          "outlayer.js"
        ]
      }
    }
  }
}
lgrassini commented 8 years ago

Thanks @EHLOVader it worked for me.