Skip to content

Commit

Permalink
fix(commonjs): correctly replace shorthand require (#764)
Browse files Browse the repository at this point in the history
* fix(commonjs): correctly replace shorthand `require`

* fix(commonjs): add comma at the replacement

* feat(commonjs): add parent type check for shorthand util
  • Loading branch information
chmelevskij committed Jan 29, 2021
1 parent 03e32d2 commit d9fb47d
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 3 deletions.
4 changes: 4 additions & 0 deletions packages/commonjs/src/ast-utils.js
Expand Up @@ -115,3 +115,7 @@ export function isLocallyShadowed(name, scope) {
}
return false;
}

export function isShorthandProperty(parent) {
return parent && parent.type === 'Property' && parent.shorthand;
}
11 changes: 8 additions & 3 deletions packages/commonjs/src/transform-commonjs.js
Expand Up @@ -11,6 +11,7 @@ import {
isDefineCompiledEsm,
isFalsy,
isReference,
isShorthandProperty,
isTruthy,
KEY_COMPILED_ESM
} from './ast-utils';
Expand Down Expand Up @@ -319,10 +320,14 @@ export default function transformCommonjs(
)}`
);
}
if (isShorthandProperty(parent)) {
magicString.appendRight(node.end, `: ${HELPERS_NAME}.commonjsRequire`);
} else {
magicString.overwrite(node.start, node.end, `${HELPERS_NAME}.commonjsRequire`, {
storeName: true
});
}

magicString.overwrite(node.start, node.end, `${HELPERS_NAME}.commonjsRequire`, {
storeName: true
});
uses.commonjsHelpers = true;
return;
case 'module':
Expand Down
@@ -0,0 +1,7 @@
const HOST = {
require
};

module.exports = {
HOST
};
11 changes: 11 additions & 0 deletions packages/commonjs/test/test.js
Expand Up @@ -726,6 +726,17 @@ test('does not wrap commonjsRegister calls in createCommonjsModule', async (t) =
t.not(/createCommonjsModule\(function/.test(code), true);
});

test('does not replace shorthand `require` property in object', async (t) => {
const bundle = await rollup({
input: 'fixtures/samples/shorthand-require/main.js',
plugins: [commonjs()]
});

const code = await getCodeFromBundle(bundle, { exports: 'named' });

t.is(/require: commonjsRequire/.test(code), true);
});

// This test uses worker threads to simulate an empty internal cache and needs at least Node 12
if (Number(/^v(\d+)/.exec(process.version)[1]) >= 12) {
test('can be cached across instances', async (t) => {
Expand Down

0 comments on commit d9fb47d

Please sign in to comment.