Skip to content
This repository was archived by the owner on Aug 4, 2021. It is now read-only.

Browser resolve fails in Node 6: opts.filename is undefined #38

Closed
nolanlawson opened this issue May 20, 2016 · 8 comments
Closed

Browser resolve fails in Node 6: opts.filename is undefined #38

nolanlawson opened this issue May 20, 2016 · 8 comments

Comments

@nolanlawson
Copy link
Contributor

This error is thrown only on Node 6, only when using rollup-plugin-node-resolve, and only when using browser: true. Those three conditions need to be met in order to reproduce this bug.

The offending line of of code is in node-browser-resolve (opts.filename is undefined), so it's possible this bug doesn't exist here but rather in that repo. I wasn't sure, so I'm filing it here.

I have a gist to reproduce.

Steps:

git clone https://gist.github.com/e029b935459d4cdb5097d1a74485be63.git gist && cd gist
npm i && node build.js

The error received is:

TypeError: Path must be a string. Received undefined
    at assertPath (path.js:7:11)
    at Object.dirname (path.js:1324:5)
    at resolve (/Users/nolan/Desktop/repro-rollup-bug/node_modules/browser-resolve/index.js:218:21)
...
@nolanlawson nolanlawson changed the title Browser resolve fails in Node 6: Browser resolve fails in Node 6: opts.filename is undefined May 20, 2016
nkoterba added a commit to nkoterba/rollup-plugin-node-resolve that referenced this issue May 25, 2016
Please see:

rollup#38
and
browserify/browser-resolve#80

for how Node v6+ throws a TypeError if `path.dirname` is passed an undefined value.  `node-browser-resolve` currently does not use set a default value if it's `opts.filename` is not specified resulting in the following stack trace:
```javascript
[19:40:45] TypeError: Path must be a string. Received undefined
    at assertPath (path.js:7:11)
    at Object.dirname (path.js:1324:5)
    at resolve (/my_app/node_modules/browser-resolve/index.js:218:21)
    at /my_app/node_modules/rollup-plugin-node-resolve/dist/rollup-plugin-node-resolve.cjs.js:46:5
    at resolveId (/my_app/node_modules/rollup-plugin-node-resolve/dist/rollup-plugin-node-resolve.cjs.js:45:11)
    at /my_app/node_modules/rollup/dist/rollup.js:2123:32
    at tryCatch (/my_app/node_modules/rollup/dist/rollup.js:393:12)
    at invokeCallback (/my_app/node_modules/rollup/dist/rollup.js:405:13)
    at publish (/my_app/node_modules/rollup/dist/rollup.js:376:7)
    at flush (/my_app/node_modules/rollup/dist/rollup.js:120:5)
```
@nkoterba
Copy link

👍 This is a critical issue we are facing as well.

In short, I believe that Node 6's internal path object no longer allows undefined inputs. Previously, I think this "bug" was overlooked by many browser-resolve users because failing to pass in a filename to browser-resolve's configuration block just silently failed and Node v5.x just happily carried on.

In short, I think this can be handled multiple ways:

  1. I think node-browser-resolve should be updated to provide a default value if filename is not passed in to the configuration block of browser-node-resolve. There is already an outstanding pull request here: Fix Node v6 browserify/browser-resolve#80 and I just made another one: Ensure a default value is set for options.filename browserify/browser-resolve#81

  2. Since the maintainer(s) of browser-node-resolve have not yet fixed this issue, also fix the rollup-plugin-node-resolve. I have submitted a pull request for this change: Ensure browserResolve has a default filename #40

@tivac
Copy link

tivac commented May 25, 2016

browserify/browser-resolve#80 was just merged, and is now available on npm as [email protected], which should match the current package.json value for browser-resolve (here) so this should be good!

@danielkcz
Copy link

I confirm that issue is indeed resolved by updating said module.

@Rich-Harris
Copy link
Contributor

Sorry, just seeing this now – am I right in thinking this and #40 can be closed if the issue has been fixed upstream? thanks

@tivac
Copy link

tivac commented Jun 7, 2016

@Rich-Harris Yes, fixed upstream. npm update should be all anyone needs.

@nkapatos
Copy link

nkapatos commented Jun 9, 2016

Hello, I am still having the same problem after the update. Do I have to manually update the dependency of browser-resolve in the package.json of the module?

└─┬ [email protected]
└── [email protected]

I suppose it is correct, right?

EDIT:
I've just seen #43 which is also my case

@Mevrael
Copy link

Mevrael commented Jun 11, 2016

@PixelVibe I've deleted node_modules folder and clean npm install worked for me.

@lammas
Copy link
Contributor

lammas commented Jun 14, 2016

@PixelVibe I'm also experiencing the same issue. Deleting node_modules and doing a clean npm install did not help.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants