search

browserify / browser-resolve

resolve function which support the browser field in package.json
MIT License
102 stars 70 forks source link

fix pathFilter for alt browser field #58

Closed mvayngrib closed 9 years ago

mvayngrib commented 9 years ago

oops, missed this one the first time around. Browserify's tests unearthed it

defunctzombie commented 9 years ago

Please add tests so it is not broken again; then I will merge.

mvayngrib commented 9 years ago

added missing tests. "deep module reference mapping (alt-browser)" detects the overlooked pathFilter bug.

also some style fixes, and added .jscsrc, idk if you want that in here

defunctzombie commented 9 years ago

What's with all the changes to the test fixtures?

mvayngrib commented 9 years ago

added "chromeapp" fields to various package.json's, added files "chromeapp" fields pull in, to differentiate from the ones pulled in by "browser" fields

mvayngrib commented 9 years ago

i'm starting to think separate tests for an alt browser field are unnecessary, so I wouldn't merge this if I were you. In fact the previous tests I added should be scrapped too, as "browser" is now nothing more than a default.

defunctzombie commented 9 years ago

K, so can we make just a test for the issue this is fixing?

On Friday, March 27, 2015, Mark Vayngrib notifications@github.com wrote:

i'm starting to think separate tests for an alt browser field are unnecessary, so I wouldn't merge this if I were you. In fact the previous tests I added should be scrapped too, as "browser" is now nothing more than a default.

— Reply to this email directly or view it on GitHub https://github.com/defunctzombie/node-browser-resolve/pull/58#issuecomment-87131805 .

mvayngrib commented 9 years ago

added, my bad dragging this out

defunctzombie commented 9 years ago

Thanks for being vigilant and following up! Fix is out in 1.8.2