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

Keep bundle order in bundle.result.json #86

Merged
merged 1 commit into from
May 23, 2016

Conversation

PlasmaPower
Copy link
Contributor

Resolves #71

@PlasmaPower
Copy link
Contributor Author

Build's failing on node <= 0.12, a bit of an odd error but I'm going to temporarily downgrade by node version to test it. I think it might be the use of Object.keys, but IDK.

@PlasmaPower
Copy link
Contributor Author

That should be fixed - for whatever reason the fs.writeFileAsync implementation wasn't getting a String, just something that looked like a String (but didn't have indexOf). Calling the String constructor on it if it isn't a string seems to have fixed it.

@chmontgomery
Copy link
Contributor

Thanks for the PR! I'm currently out of town but can take a look at this
first thing Monday
On May 20, 2016 4:05 PM, "PlasmaPower" notifications@github.com wrote:

That should be fixed - for whatever reason the fs.writeFileAsync
implementation wasn't getting a String, just something that looked like a
String (but didn't have indexOf). Calling the String constructor on it if
it isn't a string seems to have fixed it.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#86 (comment)

@PlasmaPower
Copy link
Contributor Author

@chmontgomery Sounds good!

@PlasmaPower
Copy link
Contributor Author

@AndrewCraswell does this work for your use case?

@AndrewCraswell
Copy link

@PlasmaPower, so long as this PR gets merged, it looks good! Thanks for doing the work, this will be a huge help to me and my team :).

If you'd like to also update the documentation and the existing example code to describe the enhancement, I'd be happy to Venmo or Square Cash you an extra $15. I think this is the only example you'd probably need to modify:
https://github.com/dowjones/gulp-bundle-assets/tree/master/examples/full

@PlasmaPower
Copy link
Contributor Author

@AndrewCraswell Glad I could help! I'm not sure how to describe the enhancement in any of the examples since none of them depend on the order of the generated JSON if I've looked through them correctly. I have, however, rebuilt the result JSON since that changed with my change. Is there anything else I should add there?

I've also added a note about order in the docs and readme. As for the extra $15 - thanks! If you could add it to the bounty on Bountysource or send it in Bitcoin (1MVL1ygkgRSvhxC5PwbGqdZh26XSYV2sGD) that would be great, but I just setup $PlasmaPower on Square Cash if you'd prefer that.

@PlasmaPower
Copy link
Contributor Author

@AndrewCraswell It looks like I'd have to enter in my debit card to receive money from square cash (which I'd prefer not to do for security reasons) so the $15 is yours :). Documentation should be included in a PR anyway.

@chmontgomery
Copy link
Contributor

👍 looks good to me. I have one minor update as a followup to this change so that the scripts and styles keys will always stay in order as well.

@chmontgomery chmontgomery merged commit d9b2700 into dowjones:master May 23, 2016
@PlasmaPower
Copy link
Contributor Author

@chmontgomery Makes sense - there was a race condition there.

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

Successfully merging this pull request may close these issues.

3 participants