search

ember-cli-deploy / ember-cli-deploy-revision-data

An ember-cli-plugin to create a unique revision key for the build
MIT License
21 stars 58 forks source link

Use find instead of filter to find first dist file which matches file… #37

Closed zzarcon closed 8 years ago

zzarcon commented 8 years ago

What Changed & Why

Changed filter method by find method for retrieve the first file which matches the filePattern. This way we save time since we return as soon as we find an occurrence and space since we don't want to have an array just to pick always the first value.

IMO, also I think looks more readable now. Thanks!

Related issues

PR Checklist

People

lukemelia commented 8 years ago

Thanks @zzarcon. Can you look into the failing tests? Not sure off the top of my head why your change would have caused anything to fail, but it seems to have.

zzarcon commented 8 years ago

@lukemelia maybe it's because the Node version of Travis v0.12.14 and the find method is not supported?

Works fine for me locally

There's any reason of why want to support that node version? Thanks!

achambers commented 8 years ago

Isn't Array.prototype.find an ES2015 function? We don't transpire the ember-cli-deploy code as yet so would this actually work? On Fri, 10 Jun 2016 at 15:23, Hector Zarco notifications@github.com wrote:

@lukemelia https://github.com/lukemelia maybe it's because the Node version of Travis v0.12.14 and the find method is not supported?

Works fine for me locally

https://camo.githubusercontent.com/b2a63d55e85c96032d1214a7d0fb91cb2372c349/687474703a2f2f6e65772e74696e79677261622e636f6d2f6531346332386339323032653733623566343636396334613961613665353063616265346465633038642e706e67

There's any reason of why want to support that node version? Thanks!

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ember-cli-deploy/ember-cli-deploy-revision-data/pull/37#issuecomment-225195588, or mute the thread https://github.com/notifications/unsubscribe/AAZb1J7htd224TX_aeK80jabeiNovOBMks5qKXNGgaJpZM4Iyt4y .

ghedamat commented 8 years ago

@zzarcon I'm afraid we need to support as ember-cli is still supporting node 0.10.X and up

let me know if you have time to tackle this, I'd love to get it in

zzarcon commented 8 years ago

@achambers unfortunately find is not supported on 0.12.14:

@ghedamat I think you are right, we must be aligned with the ember-cli node version :(

Just for my info, do you know why ember-cli is still supporting that version?

Thanks!

ghedamat commented 8 years ago

@zzarcon I'm honestly not entirely sure. You're probably better off asking on slack about this.

I guess we'll close this pr for now and maybe revisit later when ember cli updates

Thanks anyway!