ember-cli / broccoli-asset-rev

Broccoli plugin to add fingerprint checksums and CDN URLs to your assets
MIT License
86 stars 83 forks source link

match the functionality provided by Sprockets::Manifest #47

Closed bryanaka closed 9 years ago

bryanaka commented 9 years ago

Currently, when generateRailsManifest: true this will generate a manifest for all the assets that are spit out by broccoli. However, if also using rake assets:precompile or anything that generates a map, this will break functionality when this is the second to run.

Sprockets::Manifest actually will look for a preexisting manifest file and use that before generating a brand new file. So I've altered the functionality a bit to match that expectation.

rickharrison commented 9 years ago

Could you rebase so travis will run?

bryanaka commented 9 years ago

@rickharrison done. Thanks for working with me on this. Once things calm down at work, I'll help explore whether some kind of manifest adapter would be a useful abstraction for this plugin.

rickharrison commented 9 years ago

Thanks. Any idea why the tests are failing on travis?

bryanaka commented 9 years ago

Yeah, I was hacking the Date object to always return the same date, and it worked locally, but not on travis for some reason. The fact that fs.statSync returns a different mtime everytime the tests are run means the hash of the manifest will be different every time.

Looking into how to stub this out correctly right now.

bryanaka commented 9 years ago

If anyone has insight on how to properly mock out fs.Stats#mtime I would greatly appreciate it. I thought I could mock out the Date object or the fs.statsSync to return something deterministic, and this works locally, but no luck on travis. Also, using something like mock-fs doesn't work since broccoli creates tmp folders and is hard to deal with.

rickharrison commented 9 years ago

Have you tried using sinon?

bryanaka commented 9 years ago

yeah, no lock with sinon either.

rickharrison commented 9 years ago

Could you try having the test like this?

it('merges the generated manifest with the sprockets manifest', function(){
    var sourcePath = 'tests/fixtures/existing-manifest';

    var original = fs.statSync;
    var date = new Date(2015, 0, 1);

    sinon.stub(fs, 'statSync', function (path) {
      var stats = original.apply(this, arguments);
      stats.mtime = date;

      return stats;
    });

    var tree = assetRev(sourcePath + '/input', {
      extensions: ['js', 'css', 'png', 'jpg', 'gif', 'map'],
      replaceExtensions: ['html', 'js', 'css'],
      generateRailsManifest: true
    });

    builder = new broccoli.Builder(tree);
    return builder.build().then(function(graph) {
      confirmOutput(graph.directory, sourcePath + '/output');
    });
  });
bryanaka commented 9 years ago

Yeah even that implementation fails... if you pull that branch down, does it fail on your local machine? im at a loss here since its passing no problem locally on several machines I've tried

rickharrison commented 9 years ago

It was a timezone issue. If you construct the date like this it should work:

var date = new Date(Date.UTC(2015, 0, 1, 8));

Also, please rebase as there is now a merge conflict. Thanks!

bryanaka commented 9 years ago

Good catch :sweat:

I will rebase this and fix the conflict.

bryanaka commented 9 years ago

Sorry about the long lead time. got sidetracked by some important features at work.

Here it is, rebased, tests passing. :dancer:

bryanaka commented 9 years ago

Oh, and thanks again for figuring out that timezone issue. can't believe I didn't think of that. :beers: @rickharrison

rickharrison commented 9 years ago

Thanks!