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

Bundled dependencies built with browserify broken #77

Closed
MattiasBuelens opened this issue Jun 27, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@MattiasBuelens
Copy link
Contributor

commented Jun 27, 2016

The newest version (3.1.0) broke some of my dependencies.

For example, I bundle video.js which is built with grunt-browserify. The first line of their dist/video.js has a umd wrapper around browserify's browser-pack prelude:

(function(f){if(typeof exports==="object"&&typeof module!=="undefined"){module.exports=f()}else if(typeof define==="function"&&define.amd){define([],f)}else{var g;if(typeof window!=="undefined"){g=window}else if(typeof global!=="undefined"){g=global}else if(typeof self!=="undefined"){g=self}else{g=this}g.videojs = f()}})(function(){var define,module,exports;return (function e(t,n,r){function s(o,u){if(!n[o]){if(!t[o]){var a=typeof require=="function"&&require;if(!u&&a)return a(o,!0);if(i)return i(o,!0);var f=new Error("Cannot find module '"+o+"'");throw f.code="MODULE_NOT_FOUND",f}var l=n[o]={exports:{}};t[o][0].call(l.exports,function(e){var n=t[o][1][e];return s(n?n:e)},l,l.exports,e,t,n,r)}return n[o].exports}var i=typeof require=="function"&&require;for(var o=0;o<r.length;o++)s(r[o]);return s})({1:[function(_dereq_,module,exports){

There are two problematic lines in the browser-pack prelude:

  • var a=typeof require=="function"&&require;
  • var i=typeof require=="function"&&require;

Version 3.1.0 of this plugin transforms these lines into:

  • var a='function'=="function"&&require;
  • var i='function'=="function"&&require;

This breaks the code at run time inside a browser with strict mode enabled, since there's no global require function.

For now, I've pinned my version of rollup-plugin-commonjs to =3.0.2, so that my builds keep working.

I have not yet found the time to set up a full reproduction case, but this is the general idea:

  1. Create foo.js and bundle it with browserify. (In practice, we'd get the bundled foo.js from an npm dependency.)
  2. Create bar.js that imports from the built foo.js and bundle it with rollup.
  3. Load bar.js inside a browser.
@MattiasBuelens

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2016

I managed to set up a minimal reproduction case for this, check it out at rollup-plugin-commonjs-repro77.

Basically, the bundled code breaks on any CommonJS module built with Browserify that exports itself as a UMD module.

This is vendor/src/index.js:

exports.foo = function () {
    return 42;
};

which is built with:

browserify -s vendor -o vendor/dist/index.js vendor/src/index.js

The -s flag specifies the standalone module name, which is used to generate a UMD wrapper (similar to rollup's -n flag). In practice, the vendor/dist/index.js file would get imported from an npm module using rollup-plugin-node-resolve.

The resulting vendor module is then used in src/index.js:

import vendor from '../vendor/dist';

export function bar() {
    return vendor.foo() + 1;
}

Once again, in practice I would use import vendor from 'vendor' combined with node-resolve.

The src/index.js is bundled with rollup using:

rollup -c -o dist/index.js

with the following rollup configuration:

{
    entry: 'src/index.js',
    format: 'iife',
    moduleName: 'repro77',
    plugins: [
        commonjs({
            include: 'vendor/**'
        })
    ]
}

The resulting dist/index.jsbundle then contains the faulty 'function'=="function"&&require; which throws a reference error when executed in a browser environment.

@N1kto

This comment has been minimized.

Copy link

commented Jul 12, 2016

@MattiasBuelens thanks a lot :) Spent about 2 hours today trying to figure what was wrong with those magic lines:
var a='function'=="function"&&require;
var i='function'=="function"&&require;

inside jszip. Then I recalled that I recently upgraded commonjs plugin ...

@MattiasBuelens

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2016

While waiting for a fix, I have a (really dirty) workaround for this. The problem is that the browserify code tries to access a non-existing require variable, which causes a ReferenceError - so let's just declare our own require variable inside the bundle! It's even quite easy using a rollup plugins with an intro hook:

{
    plugins: [
        nodeResolve(),
        commonjs({
            include: 'node_modules/**'
        }),
        {
            intro: function() {
                return 'var require;';
            }
        }
    ]
}

This makes the require inside the browserify dependency reference our bundle-scoped variable, rather than ending up in the global scope. The browserify code also checks that require is truthy before it tries to use it, so our require can just be undefined. Of course, this wouldn't work in case some other CommonJS dependency doesn't do this extra check, for example this would fail:

if (typeof require === 'function') {
   var fs = require('fs'); // undefined is not a function
}

It's good enough for my use case though. 😛

In the meantime, any ideas on how this should be fixed "properly"? What should this plugin do with browserify dependencies?

Rich-Harris added a commit that referenced this issue Sep 17, 2016

Rich-Harris added a commit that referenced this issue Sep 17, 2016

Merge pull request #108 from rollup/gh-77
fix typeof require === "function" && require
@Rich-Harris

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2016

Fixed (properly) in 5.0.2, sorry for the wait!

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.