Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8.1.1 fails to resolve modules from "browser" field #1072

Closed
omsmith opened this issue Jan 16, 2015 · 13 comments
Closed

8.1.1 fails to resolve modules from "browser" field #1072

omsmith opened this issue Jan 16, 2015 · 13 comments

Comments

@omsmith
Copy link

omsmith commented Jan 16, 2015

Minimal reproduction: https://gist.github.com/omsmith/a6b8876a4354960a65f4

Minimal reproduction: https://github.com/omsmith/browserify-8.1.1-resolve-issue

Works fine in 8.1.0, but as of 8.1.1 browser entries don't appear to be picked up

@omsmith
Copy link
Author

omsmith commented Jan 16, 2015

Hmm I'm a bit of a doofus and my example is broken for other reasons - will link an updated one in a moment.

@omsmith omsmith closed this as completed Jan 16, 2015
@omsmith
Copy link
Author

omsmith commented Jan 16, 2015

Alright, issue is present in this example repo.

Doesn't occur when being required within the bundle (as browser-resolve is used then), but occurs when requiring directly (b.require('thing-in-browser-field')).

> b-8.1.1@1.0.0 build /home/osmith/dev/b-8.1.1
> ./build.sh

============ STARTING MAIN BUNDLE =============
============ STARTING VENDOR BUNDLE =============
Error: Cannot find module 'some-lib' from '/home/osmith/dev/b-8.1.1'
    at /home/osmith/dev/b-8.1.1/node_modules/browserify/node_modules/resolve/lib/async.js:50:17
    at process (/home/osmith/dev/b-8.1.1/node_modules/browserify/node_modules/resolve/lib/async.js:120:43)
    at /home/osmith/dev/b-8.1.1/node_modules/browserify/node_modules/resolve/lib/async.js:129:21
    at load (/home/osmith/dev/b-8.1.1/node_modules/browserify/node_modules/resolve/lib/async.js:60:43)
    at /home/osmith/dev/b-8.1.1/node_modules/browserify/node_modules/resolve/lib/async.js:66:22
    at /home/osmith/dev/b-8.1.1/node_modules/browserify/node_modules/resolve/lib/async.js:21:47
    at Object.oncomplete (fs.js:97:15)

@omsmith omsmith reopened this Jan 16, 2015
@jmm
Copy link
Collaborator

jmm commented Jan 16, 2015

According to the documentation for b.require(file, opts), "The file param is anything that can be resolved by require.resolve()." So if it's intended to be able to do what you're trying to do, that documentation needs an update. I do think there are currently a lot of issues around what values are acceptable arguments for browserify(), .add(), .require(), and .external(). See #1039, #1059, #1076.

That's not even really what the browser field is for anyway though, is it? If you're doing browserify -r or b.require() why not just use the path to the actual file you want?

If you could use the API instead of the CLI, then at least for the time being (until you see how this shakes out) you could do this, right?:

 b.require(require('browserify/package.json').browser.some-lib)

@omsmith
Copy link
Author

omsmith commented Jan 17, 2015

It may not be exactly what the browser it's meant for, no.

In the actual case, the errors being run into here were for shimming angular modules.

Then the list was then passed as external to the main bundle, and passed as requires to our vendor bundle.

Given the need for the proper names to be exposed here, the fix (without browserify changes) would actually be to, similar to what you said, require the filename directly and specify the name to expose b.require('src/browser if he's home/angular.js', { expose: 'angular' }).

This did however, work before - only broken by the use of the node resolve, which would likely return to normal by changing resolve(file, to bresolve(file,. Just haven't yet tested such.

@walterhiggins
Copy link

I think the fix here is to first try to bresolve() the file and only if bresolve() fails then resolve() the file.
I would expect that the browser: config in package.json would always take precendence over the dependencies: config.

8.1.0...master#diff-168726dbe96b3ce427e7fedce31bb0bcR155

@jmm
Copy link
Collaborator

jmm commented Jan 19, 2015

This did however, work before

But intentionally or accidentally? That usage doesn't seem to be documented and apparently there wasn't test coverage for it either.

@omsmith

would likely return to normal by changing resolve(file, to bresolve(file,...

@walterhiggins

I think the fix here is to...

I've proposed (#1077) eliminating that pathname resolution that was added recently, reverting to doing it in module-deps.

first try to bresolve the file and only if bresolve fails then resolve the file

Isn't bresolve a superset of resolve?

@baseten
Copy link

baseten commented Feb 8, 2015

I'm having a problem with this too, whether or not it was an intentional behaviour. My use case is for building separate bundles for library and application code. It was working by setting up named entries in the browser field (a mix of shimmed and native common-js libraries), then when building I would pass an array of these names to the library bundle's require function as well as the application bundle's external function. I can provide a minimal example if this is useful. I've been unable to find another way to achieve the same thing in versions newer than 8.1.0

@omsmith
Copy link
Author

omsmith commented Feb 8, 2015

I've not checked, but I expected the changes in 8.1.2 would've fixed this. Does it not @baseten?

@baseten
Copy link

baseten commented Feb 9, 2015

@omsmith it doesn't appear to. I'm still getting the same error in 8.1.2 and 8.1.3

@justinware
Copy link

Yeah, I am also getting this issue now with 8.1.3. Was on 5.x as hadn't upgraded for a while, and now that I'm on the latest version it is broken as per @omsmith & @baseten's experiences. I too was creating a separate vendor / library module bundle by passing the aliases defined in package.json into the .require() function during my gulp bundle task. Am doing this instead of just doing a .require() on the relative path for the vendor libs as I need browserify-shim to kick in for the Global namespacing adjustments.

  "browser": {
    "rxjs": "./public/dist/rxjs/dist/rx.all.js"
  },
  "browserify": {
    "transform": [
      "browserify-shim"
    ]
  },
  "browserify-shim": {
    "rxjs": "Rx"
  },

Then...

b.require('rxjs');

now gives the Cannot find module error.

I am happy to try a few workarounds, like the one suggested:

b.require('./public/dist/rxjs/dist/rx.all.js', { expose: 'rxjs' })

...however does anyone know if browserify-shim will kick-in from package.json and set the Global namespace objects accordingly or if package.json is ignored entirely when doing a direct .require() on the file path ??

@nkoterba
Copy link

@justinware I can confirm that doing

b.require('./node_modules/angular/angular.js', {expose: 'angular'});

does indeed work. However, I too am trying to figure out what, if anything, browserify-shim will do from package.json. I'm seeing very odd behavior. For some of my shims (kendo UI, which depends: jQuery), I'm seeing jQuery pulled in by browserify-shim without explicitly calling b.require('jquery'). However, for other shims (like angular which depends: jQuery), jQuery is Not pulled in. Very confusing and odd.

@ghost
Copy link

ghost commented Feb 21, 2015

The changes upstream in resolve@1.1.4 and module-deps appear to have fixed this issue.
Using the reproduction repo with browserify@9.0.1 I now get no errors:

/tmp/browserify-8.1.1-resolve-issue $ npm run build

> b-8.1.1@1.0.0 build /tmp/browserify-8.1.1-resolve-issue
> ./build.sh

============ STARTING MAIN BUNDLE =============
============ STARTING VENDOR BUNDLE =============

@ghost ghost closed this as completed Feb 21, 2015
@omsmith
Copy link
Author

omsmith commented Feb 21, 2015

Very cool - thanks @substack

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

6 participants