Reuse existing manifest option#23
Conversation
|
@phamann I hope you don't mind but i'm going to publish my own branched version under a new namespace until this is merged because it's a little time sensitive for me. If there is an issue i'm willing to unpublish. |
|
It would be good to see it merged, along with other working PRs. I've reviewed this one and I'm OK for it, if you care about my opinion. The PR is very targeted & simply working well. |
|
@phamann could we get a quick approval or a review? I'd like to get this PR off my PR list. |
|
@GerkinDev By the way, if you want to use a forked version with this code I published a fork here: https://www.npmjs.com/package/rollup-plugin-hash-manifest |
|
@monsonjeremy Hi Jeremy, thanks for implementing this! 👍 |
| const res = hashWithOptions({ dest: 'tmp/[hash].js', manifest: 'tmp/manifest.json' }); | ||
| return res.then(() => { | ||
| const tmp = fs.readdirSync('tmp'); | ||
| const manifest = require('./tmp/manifest.json'); |
There was a problem hiding this comment.
Not sure what you mean here.
There was a problem hiding this comment.
I don't see what improvements or changes this diff does. require already parses the JSON.
There was a problem hiding this comment.
@GerkinDev, looks like this wasn't a useless chore. Without these, the tests fail on Node 4 since require likely does not parse it in that old node version.
There was a problem hiding this comment.
Okay my bad. But since you changed tested node env (see this diff), is it still relevant ?
There was a problem hiding this comment.
Good call, removed those changes. Should be good to merge now!
|
@GerkinDev I've made all the requested changes, do you mind merging this? |
|
@monsonjeremy I'm not a maintainer of this package ^^ I just reviewed to help, and validated your changes. I really can't do anything more. @phamann ping ! |
|
@GerkinDev Sorry, thought you were a maintainer! Anyways, i'll give it another week and then i'm going to delete the PR. |
|
Dammit. |
3 participants
Piggy-backing off the work done in this PR to reuse the existing manifest. I also added this as an optional flag since it's not desired for all users of this plugin.