search

gulpjs / liftoff

Launch your command line tool with ease.
MIT License
843 stars 52 forks source link

Set modulePath correctly #33

Closed callumlocke closed 9 years ago

callumlocke commented 9 years ago

fixes #31

tkellen commented 9 years ago

Thanks @callumlocke! I'm going to look at this closer later today. Can you add a test case for this? Hopefully one that would pass in 1x but fail in 2x without your additions.

callumlocke commented 9 years ago

I was actually wrong that this was a bug introduced in v2. I just reverted to v1 and had the same problem. Nothing else changed in my project so I'm not sure what caused my code to stop working... Could be something to do with me upgrading Node. I don't know.

However... I think my change is probably what you meant to write anyway :) It fixes an edge case where you are testing a module that uses Liftoff, and it loads 'itself', but you're testing from a CWD that is not the root of your project, but something deeper (eg. ./test/). Your code already checked for this edge case when requiring the module's package.json, but it didn't do the same check when constructing the module path, so the two were not in sync.

When you have a closer look at this PR's diff, let me know if you still think it needs a test... personally I would say it's not the kind of bug that needs a regression test.

tkellen commented 9 years ago

Nice catch. This definitely needs a test! If you can add one I'll merge and cut a release.

callumlocke commented 9 years ago

Currently your tests don't cover the "developing against ourselves" situation at all. In fact you can comment out the entire block at L95-L107, and all your tests still pass. My PR just changes a couple of lines within that already-untested block of code.

If you think the whole block needs a test, I think that's a separate issue, and I don't know a good way to go about testing it.

tkellen commented 9 years ago

Fair enough!

tkellen commented 9 years ago

Published as 2.0.1, would really appreciate a PR against https://github.com/tkellen/node-liftoff/issues/34 if you find the time.