Skip to content

Commit

Permalink
Strip no-side-effect imports from Rollup bundles
Browse files Browse the repository at this point in the history
  • Loading branch information
Brian Vaughn committed Aug 29, 2018
1 parent 0f805c3 commit 3021a81
Showing 1 changed file with 30 additions and 0 deletions.
30 changes: 30 additions & 0 deletions scripts/rollup/build.js
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,33 @@ function shouldSkipBundle(bundle, bundleType) {
return false;
}

// Strip unused require() statements for pure externals modules from the bundle.
// Rollup's treeshake option should do this, but it doesn't work.

This comment has been minimized.

Copy link
@gaearon

gaearon Aug 29, 2018

Collaborator

Have we figured out why? It might be that we need to update Rollup to get some upstream bugfix. Or maybe there's something simple we can fix ourselves and send it to them.

This comment has been minimized.

Copy link
@bvaughn

bvaughn Aug 29, 2018

Contributor

Thanks for the links! 😄

Unfortunately I believe these tests are for the latest release of Rollup. As I mentioned in my other comment, we're 13 versions behind.

The version we're using does not work with boolean, array-of-strings, map of key-to-boolean, nor function.

This comment has been minimized.

Copy link
@TrySound

TrySound Aug 29, 2018

Contributor

Then it's a good chance to migrate. I believe this is the only blocker
#13356

This comment has been minimized.

Copy link
@bvaughn

bvaughn Aug 29, 2018

Contributor

As I also mentioned in my other comment on this PR, we are prevented from upgrading by a circular dependency that exists.

This comment has been minimized.

Copy link
@TrySound

TrySound Aug 29, 2018

Contributor

Then it would be better to move this hack into rollup plugin to prevent extra fs accesses

{
  transformBundle(code) {
    const isProduction = isProductionBundleType(bundleType);
     // Dead code elimintation is only applied to production bundles.
    if (!isProduction) {
      return;
    }
     let codeOut = code;
     pureExternalModules.forEach(module => {
      const regExp = new RegExp(
        `(?<!= *)require\\(["']${module}["']\\)[,;]`,
        'g'
      );
      codeOut = codeOut.replace(regExp, '');
    });
    return { code }
  }
}
function stripNoSideEffectImports(bundleType, pureExternalModules, filePath) {
const isProduction = isProductionBundleType(bundleType);

// Dead code elimintation is only applied to production bundles.
if (!isProduction) {
return;
}

const codeIn = fs.readFileSync(filePath, 'utf-8');
let codeOut = codeIn;

pureExternalModules.forEach(module => {
const regExp = new RegExp(
`(?<!= *)require\\(["']${module}["']\\)[,;]`,
'g'
);

codeOut = codeOut.replace(regExp, '');
});

if (codeIn !== codeOut) {
fs.writeFileSync(filePath, codeOut, 'utf-8');
}
}

async function createBundle(bundle, bundleType) {
if (shouldSkipBundle(bundle, bundleType)) {
return;
Expand Down Expand Up @@ -493,6 +520,9 @@ async function createBundle(bundle, bundleType) {
try {
const result = await rollup(rollupConfig);
await result.write(rollupOutputOptions);

// HACK to work around the fact that Rollup isn't removing unused, pure-module imports.
stripNoSideEffectImports(bundleType, pureExternalModules, mainOutputPath);
} catch (error) {
console.log(`${chalk.bgRed.black(' OH NOES! ')} ${logKey}\n`);
handleRollupError(error);
Expand Down

0 comments on commit 3021a81

Please sign in to comment.