search

es128 / progeny

:baby: Recursively finds dependencies of style and template source files
MIT License
29 stars 15 forks source link

progeny.Sync outputs an empty string on Stylus glob patterns #16

Closed diagramatics closed 8 years ago

diagramatics commented 8 years ago

To replicate here's a unit test:

it 'should resolve glob patterns', ->
  dependencies = progeny.Sync() getFixturePath('globbing.styl')
  paths = (getFixturePath x for x in ['base/glob/styles1.styl', 'base/glob/styles2.styl'])
  assert.deepEqual dependencies, paths

It will always output an empty string.

es128 commented 8 years ago

Thanks for reporting. Pretty clear what's going on - https://github.com/es128/progeny/blob/master/src/index.coffee#L132-L136 needs to factor in whether it's in Sync or Async mode and use glob.sync as appropriate (potentially wrapped using https://github.com/es128/cbify to avoid needing as much refactoring here).

PR welcome (including that unit test). Otherwise I'll try to take care of it myself, but my general OSS activity level has been low lately so I can't give any assurances about timing.

diagramatics commented 8 years ago

I could always wrap the loop for calling addDep into a function expression outside of the conditional to check for the Sync / Async mode too.

It's ultimately your call, but I've done what I mentioned and I'll submit a PR soon.

es128 commented 8 years ago

Yup, the way you did it should be fine.

The cbify/fs-mode solution was convenient for not having to do something similar for every single fs call. But since there's only one call to glob, no point in trying to do anything fancier.

Looks like we're ready to publish 0.7. WDYT?

es128 commented 8 years ago

Or even 1.0 maybe... I'd just want to convert to vanilla ES5 first before calling it 1.0, otherwise seems alright as-is.

diagramatics commented 8 years ago

I'm in favor of releasing 0.6.1 since it is just fixes to things even. Technically we can replace that bit with a Promise chain too if we have ES6 support.

es128 commented 8 years ago

Hmm, yes I suppose you're right that all these changes are bug fixes. I'll plan to publish as 0.6.1 tomorrow.

We need to stick with ES5 compatibility for now, but maybe we could consider going to ES6 later along with a major version bump.

diagramatics commented 8 years ago

We could always set up Babel for it, of course.

es128 commented 8 years ago

Sure, but meh - not worth it here imo. There's not that much happening in this repo, would rather keep it simple.