ForbesLindesay / browserify-middleware

express middleware for browserify, done right
http://browserify.org
MIT License
381 stars 66 forks source link

Fix dynamic caching to prevent rebuilding the whole bundle on every load #75

Closed scottbrady closed 9 years ago

scottbrady commented 9 years ago

This should fix https://github.com/ForbesLindesay/browserify-middleware/issues/72 and https://github.com/ForbesLindesay/browserify-middleware/issues/68

allain commented 9 years ago

I've installed it locally, ran the tests and it all passes (after changing the timeout for all of the tests to 5 seconds).

Patch itself looks good, I'd probably write getDeps as the following which is equivalent to Object.values, but it's just style.

return Object.keys(dep.deps).map(function (key) {
    return dep.deps[key];
});

Nice work!

ForbesLindesay commented 9 years ago

I agree we should re-write getDeps to look like Object.values. I'll fix that up after merge though.

@scottbrady: Would you like to be added as a collaborator on this project, to help review and merge pull requests?

scottbrady commented 9 years ago

Thanks for merging the patch so quickly. I'd be happy to help out with pull requests! Cheers.

scottbrady commented 9 years ago

I installed 5.0.1 and I'm now getting this error:

TypeError: Object.keys called on non-object
  at Function.keys (native)
  at objectValues (node_modules/browserify-middleware/lib/dynamic-cache.js:7:17)
  at DynamicCache.<anonymous> (node_modules/browserify-middleware/lib/dynamic-cache.js:83:35)
  at Object.oncomplete (fs.js:108:15)

I was able to fix the error by adding a type check to the objectValues function:

function objectValues(obj) {
  if (typeof obj !== 'object') {
    return [];
  }
  return Object.keys(obj).map(function (key) {
    return obj[key];
  });
}

Want me to submit another patch or can you take care of it?

Thanks.

allain commented 9 years ago

done

scottbrady commented 9 years ago

Thanks @allain!

allain commented 9 years ago

@ForbesLindesay Given that this has been fixed can you bump the version on npm?

allain commented 9 years ago

When you bring in contributors, you really mean it. I just pushed to npm successfully.

ForbesLindesay commented 9 years ago

@scottbrady just saw that I forgot to actually add you as a collaborator. Should be fixed now :)

Please follow these rules:

  1. never commit directly to master unless you're only editing documentation, or you're in the process of cutting a new release.
  2. never merge your own pull request unless it's had no comments from any collaborators for at least 24 hours.
  3. If you cut a new release, make sure you update History.md and follow semver when updating the version number.

If you let me know your npm username I'll add you as an owner so you can release new versions :)

@allain Absolutely :+1:. Without help from people like you, these projects would eventually wither and die from lack of my time.

scottbrady commented 9 years ago

@ForbesLindesay I'm scottbrady on npm https://www.npmjs.com/~scottbrady

ForbesLindesay commented 9 years ago

Great. I've added you on npm :)