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

Broken in node@v6.x.x #63

Closed
tivac opened this issue May 10, 2016 · 7 comments

Comments

Projects
None yet
5 participants
@tivac
Copy link
Contributor

commented May 10, 2016

rollup-plugin-commonjs is broken in node@6.x.x due to the path module throwing if any of its input is not a string.

A small repro can be found here: https://gist.github.com/tivac/f4f8b4086fb8bc0c9d1cd166f1a2b396

> rollup -config ./rollup.config.js -i ./index.js

TypeError: Path must be a string. Received undefined
    at assertPath (path.js:7:11)
    at Object.dirname (path.js:697:5)
    at resolveId (C:\Users\pcavit\Desktop\rollup-cjs\node_modules\rollup-plugin-commonjs\dist\rollup-plugin-commonjs.cjs.js:103:37)
    at C:\Users\pcavit\Desktop\rollup-cjs\node_modules\rollup\Volumes\Storage\www\ROLLUP\rollup\src\utils\first.js:10:31
    at tryCatch (C:\Users\pcavit\Desktop\rollup-cjs\node_modules\rollup\Volumes\Storage\www\ROLLUP\rollup\node_modules\es6-promise\lib\es6-promise\-internal.js:185:12)
    at invokeCallback (C:\Users\pcavit\Desktop\rollup-cjs\node_modules\rollup\Volumes\Storage\www\ROLLUP\rollup\node_modules\es6-promise\lib\es6-promise\-internal.js:197:13)
    at publish (C:\Users\pcavit\Desktop\rollup-cjs\node_modules\rollup\Volumes\Storage\www\ROLLUP\rollup\node_modules\es6-promise\lib\es6-promise\-internal.js:168:7)
    at flush (C:\Users\pcavit\Desktop\rollup-cjs\node_modules\rollup\Volumes\Storage\www\ROLLUP\rollup\node_modules\es6-promise\lib\es6-promise\asap.js:87:5)
    at _combinedTickCallback (internal/process/next_tick.js:67:7)
    at process._tickCallback (internal/process/next_tick.js:98:9)

/src/index.js#L69 is causing the issue, I haven't dug in deeper yet.

@tivac

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2016

I'm on windows, fwiw. Looking to see why tests aren't failing in node6 atm.

@tivac

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2016

Ok, I see why this is failing for me but not in the tests.

My repo invokes rollup like this: rollup -config ./rollup.config.js -i ./index.js

The first time the resolveId hook is called it gets passed "./index.js" as the importee and undefined as the importer. It verifies that the importee begins with a "." and then tries to resolve the dirname of undefined which explodes.

The tests don't run into this because their entry value never starts with a ".". I can prevent this bug by invoking rollup like this: rollup -config ./rollup.config.js -i index.js

But that seems really unexpected and wrong. I think the check in /src/index.js#L67 should be changed to:

if ( importee[0] !== '.' || !importer ) return; // not our problem

Thoughts? All the tests still pass w/ that change, because it's only true for the entry module which should always be an ES Module (I assume).

tivac added a commit to tivac/rollup-plugin-commonjs that referenced this issue May 10, 2016

@Flaise

This comment has been minimized.

Copy link

commented Jun 22, 2016

Same issue with Mac OS X and Node 6.

@Rich-Harris

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2016

Fixed in 3.1.0, thanks for the PR @tivac

@barneycarroll

This comment has been minimized.

Copy link

commented Jul 8, 2016

I'm getting this on node 4.4.5:

$ rollup --config --watch

checking rollup-watch version...
bundling...
Path must be a string. Received undefined
TypeError: Path must be a string. Received undefined
    at assertPath (path.js:8:11)
    at Object.win32.resolve (path.js:130:5)
    at resolveId$1 (C:\Users\ttcarrollb\git\mithril-inspector\node_modules\rollup-plugin-node-resolve\dist\rollup-plugin-node-resolve.cjs.js:44:15)
    at C:\Users\ttcarrollb\git\mithril-inspector\node_modules\rollup\src\utils\first.js:8:31
    at process._tickCallback (node.js:369:9)
Type rollup --help for help, or visit https://github.com/rollup/rollup/wiki

My rollup.config.js:

module.exports = {
    entry      : './src/index.js',
    dest       : './build/index.js',
    format     : 'iife',
    sourceMap  : true,
    plugins    : [
        require( 'rollup-plugin-buble' )( {
            transforms : {
                dangerousForOf                : true,
                dangerousTaggedTemplateString : true
            }
        } ),
        require( 'rollup-plugin-commonjs' )( {
            include : './node_modules/**'
        } ),
        require( 'rollup-plugin-node-resolve' )( {
            jsnext  : true,
            main    : true
        } )
    ]
}

My pacakge.json's devDependencies:

{
    "rollup": "0.33.1",
    "rollup-plugin-buble": "0.12.1",
    "rollup-plugin-commonjs": "3.1.0",
    "rollup-plugin-node-resolve": "1.7.1",
    "rollup-watch": "2.4.0"
}

EDIT: locked down the version numbers, removed node_modules, cache cleaned, reinstalled, reproduce

Any ideas?

@lammas

This comment has been minimized.

Copy link

commented Jul 8, 2016

That error is coming from the rollup-plugin-node-resolve module. It'll work once rollup/rollup-plugin-node-resolve#45 is merged, but until then you can work around it by removing './' from the entry filename.

@barneycarroll

This comment has been minimized.

Copy link

commented Jul 8, 2016

Thanks @lammas — I spotted the error just as you posted. Confirmed fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.