Skip to content

Conversation

mvayngrib
Copy link
Contributor

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

@@ -180,17 +180,17 @@ function build_resolve_opts(opts, base) {
if(mappedPath) {
return mappedPath;
}
if(!info.browser) {

var replacements = info[browser];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syntax isssue

@defunctzombie
Copy link
Collaborator

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

@mvayngrib
Copy link
Contributor Author

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
Copy link
Collaborator

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

@mvayngrib
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
#58 (comment)
.

@mvayngrib
Copy link
Contributor Author

added, my bad dragging this out

defunctzombie added a commit that referenced this pull request Mar 28, 2015
fix pathFilter for alt browser field
@defunctzombie defunctzombie merged commit d4cd26f into browserify:master Mar 28, 2015
@defunctzombie
Copy link
Collaborator

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

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.

2 participants