Cannot read property 'warn' of undefined when preferBuiltins is not set

[simonhaenisch]

rollup-plugin-node-resolve/src/index.js

Lines 177 to 183 in 73b01b1

	if ( !isPreferBuiltinsSet ) {
	this.warn(
	`preferring built-in module '${importee}' over local alternative ` +
	`at '${resolved}', pass 'preferBuiltins: false' to disable this ` +
	`behavior or 'preferBuiltins: true' to disable this warning`
	);
	}

When not having preferBuiltins set, I have a case where this piece of code throws an error

Plugin Error: Cannot read property 'warn' of undefined

Can't give a reproducible example right now but just wondering whether this is considered a bug, or whether I'm doing something wrong to have this (the plugin context) being undefined? I just changed the resolveId hook to log if this === undefined and there were heaps of modules where that was the case, so I'm wondering whether there should be a 'warn' in this check before trying to throw a warning?

As a workaround, setting preferBuiltins (to either true or false) does the trick.

Edit: The package/importee that caused this was events as a dependency of @featherjs/feathers, and the resolveId hook was called three times. The first time this was working and the warning came through, but then it was called again and that's when this was undefined...

This is the error being thrown:

{ TypeError: Cannot read property 'warn' of undefined
    at <line number that refers to the quoted code above>
    at <anonymous> code: 'PLUGIN_ERROR', plugin: 'commonjs', hook: 'resolveId' }

So I think the calls actually came from rollup-plugin-commonjs (does it use node-resolve?), and maybe that's why this isn't set within node-resolve, because the context is a different plugin (commonjs)?

[lukastaegert]

Just to check, are you using a fairly recent Rollup version? The plugin context for resolveId has not been around forever, I believe it was added in Rollup@0.60.0

[simonhaenisch]

Yes, that's with

• rollup@1.1.0
• rollup-plugin-node-resolve@4.0.0
• rollup-plugin-commonjs@9.2.0

[bitflower]

@lukastaegert anything else that comes to your mind? This came up in StencilJS project after almost a year of deployment without any issues. One of the changes in Stencil was actually the rollup update recently as @simonhaenisch already pointed out.

[lukastaegert]

There is in fact an issue with rollup-plugin-commonjs. During initialization, rollup-plugin-commonjs directly calls rollup-plugin-node-resolve in order to resolve the namedExports option and it is not passing in any context. Which is understandable as at this time, there is no context available. I guess rollup-plugin-commonjs should pass in a dummy handler that just logs to the console.

UPDATE: In fact, it is not during initialization but later. Fix pending.

[simonhaenisch]

@lukastaegert I had a look at your PR...

candidate.call(this, ...args)

That means the this context is passed on from the calling plugin, right? Wouldn't that cause the warning to be thrown twice (or more) if preferBuiltins is not set? Or does the queue throw out the warning if it's a duplicate?

[cellog]

I am running into this bug, with no namedExports

[!] (commonjs plugin) TypeError: Cannot read property 'warn' of undefined
TypeError: Cannot read property 'warn' of undefined
    at node_modules/rollup-plugin-node-resolve/dist/rollup-plugin-node-resolve.cjs.js:186:16
    at <anonymous>

(stripped full path for readability)

			return resolveIdAsync(
				importee,
				Object.assign( resolveOptions, customResolveOptions )
			)
				.catch(function () { return false; })
				.then(function (resolved) {
					if (options.browser && packageBrowserField) {
						if (packageBrowserField[ resolved ]) {
							resolved = packageBrowserField[ resolved ];
						}
						browserMapCache[resolved] = packageBrowserField;
					}

					if ( !disregardResult && resolved !== false ) {
						if ( !preserveSymlinks && resolved && fs.existsSync( resolved ) ) {
							resolved = fs.realpathSync( resolved );
						}

						if ( ~builtins.indexOf( resolved ) ) {
							return null;
						} else if ( ~builtins.indexOf( importee ) && preferBuiltins ) {
							if ( !isPreferBuiltinsSet ) {
								this$1.warn(
									"preferring built-in module '" + importee + "' over local alternative " +
									"at '" + resolved + "', pass 'preferBuiltins: false' to disable this " +
									"behavior or 'preferBuiltins: true' to disable this warning"
								);
							}
							return null;

Line 186 is this:

								this$1.warn(
									"preferring built-in module '" + importee + "' over local alternative " +
									"at '" + resolved + "', pass 'preferBuiltins: false' to disable this " +
									"behavior or 'preferBuiltins: true' to disable this warning"
								);

adding an explicit preferBuiltins removes the crash, but the larger issue is that this is not set in the then

[simonhaenisch]

@cellog you're referencing the same thing but the dist version rather than the source... there is already a PR that's fixing the missing this context (rollup/rollup-plugin-commonjs#370). I suggest you explicitly set preferBuiltins until the patch is merged and released.