Browserify v5 support, update tests, and add some comments

[AlexRiedler]

Update tests so they all pass for browserify v5.10+ ; note that fullpaths: true must occur (there is probably a way around this but we can do it in a future version). Note this also version bumps to v4.0.0 respectively.

require(entry) does not apply transformations anymore in browserify for some reason; I am not sure if this is a bug or intended behaviour, that is why I moved the entry points into the main browserify function in tests. Fixes #78

[thlorenz]

Thanks, looks good, I'll be happy to merge this once the above changes were done (basically include only changes necessary to fix the issue) and below questions clarified.

I'm not sure how the fullPath option will work when this is run from the commandline?

How will it work when the package is used as a library and has the transform applied automatically solely based on information inside the package.json.
Is there a way to configure the fullPath option in the package.json as well?

[AlexRiedler]

@thlorenz there is a --full-paths options on the command line (updated the README.md to reflect)

I am not sure if there is a way to configure fullPaths from package.json; as far as I am aware you cannot set browserify options in package.json (unless I am mistaken).

All changes also have been completed in your regard to comments.

[thlorenz]

@AlexRiedler if we cannot set this option from package.json that would mean it would break for packages that are used as libraries and apply the shim transform when required.
However I basically created version 3 of b-shim originally to enable exactly that feature.

Could you create an issue in browserify to find out if it can be configured (link this PR there).
If it cannot be configured currently we should try to get that fixed since as we can see here it is important to be able to do so.

[osama-lionheart]

@AlexRiedler, thanks for submitting this, I was trying to QA it using this fork but it still can't find the angluar module, can you please take a look at that repo.

Context: The original repo is working with browserify v3.41.0 and browserify-shim v3.4.1, in the above fork I've update browserify to v5.10.0 and browserify-shim to your latest commit 4ae5b5c and added --full-paths

Let me know if I've missed something to make it work.

[AlexRiedler]

@osama-lionheart it looks like the nested dependency is failing, as in angular-route requires angular and for some reason cannot find it... (looking at stack trace currently).

It seems that it is using the other require and not the browserify_shim_require for some reason ; that is as far as I got debugging it before I started work today.

[thlorenz]

@osama-lionheart did this work with an older version of browserify?

@AlexRiedler currently on vacation, so will have a look on the weekend. Wanna explore the possibility of resolving the paths instead of depending on fullPath option.

[osama-lionheart]

@thlorenz, yes

The original repo is working with browserify v3.41.0 and browserify-shim v3.4.1

[thlorenz]

I got this to work with and without fullPaths. Only the tests needed to be adapted.

I'm also testing against all major browserify versions now.

So closing this PR since not relying on fullPaths at all times is the better option.

@osama-lionheart can you please test against the next version of b-shim?

It is published as v3.7.0.

[osama-lionheart]

@thlorenz, I just tested against the test repo that I forked and I'm still experiencing the same issue:

Uncaught Error: Cannot find module '<path>/grunt-browserify-angular-boilerplate/bower_components/angular/angular.js' 

As far as I can see from the commits, you've fixed the tests (which fixed the warning that we were getting while installing this against browserify v5+) but I don't think it addresses the original issue reproduced in the above test repo.

Is there anything I can do to help debugging this?

[thlorenz]

That is odd since none of the browserify-shim code changed from one version to the next (only the tests).
So I think this could be a browserify issue instead.

[osama-lionheart]

none of the browserify-shim code changed from one version to the next (only the tests)

yes, that's my point, I think we had two issues one is to fix the tests, which I think is now resolved, but another one is to fix functionality described in #40 but against browserify v5, which is what the test repo is trying to test actually.

[thlorenz]

@osama-lionheart just to clarify if you use older browserify version everything works fine right?

It'd be very helpful to generate a smaller test case from the sample repo, i.e. like here. Seems to me it'd be similar to the multideps test.
Obviously don't check in entire angular lib, just generate stubs for each instead like I did for jquery here and for ember here.

That way we have a failing test and can work on fixing that.

[osama-lionheart]

@thlorenz, yes, using browserify@4 everything works fine, but the issue is against browserify@5.

I've created #84 to add a new test that should help getting this fixed.

[thlorenz]

Thanks, I'll have a look, but if I understand you correctly, the change has to be applied to browserify right?
So we'll just wait til that happens and then the test you added to b-shim should pass?

[osama-lionheart]

@thlorenz, actually I thought the issue was with browserify-shim, but I've just created a commonjs files and tried to use the same multiple bundles technique, and I found that the issue is with browserify itself, so no work is required for this here.

Also I've found the issue reported here: browserify/browserify#860

[thlorenz]

Thank you. I'll hold off on merging your PR with the test until the issue with browserify is solved.
Then the passing test will prove that it's actually fixed and stay that way.

[osama-lionheart]

Actually I found out how to do it correctly in browserify v5, we just needed to use the expose property, please see my update pull-request, now the test is passing against both v4 and v5.