Skip to content

Commit

Permalink
fix: conditional require to not have side effects (#4)
Browse files Browse the repository at this point in the history
  • Loading branch information
privatenumber committed Apr 29, 2022
1 parent 9cbdad8 commit b9c936c
Show file tree
Hide file tree
Showing 6 changed files with 37 additions and 19 deletions.
2 changes: 1 addition & 1 deletion package.json
Expand Up @@ -55,7 +55,7 @@
},
"dependencies": {
"@rollup/plugin-alias": "^3.1.9",
"@rollup/plugin-commonjs": "^21.1.0",
"@rollup/plugin-commonjs": "^22.0.0",
"@rollup/plugin-inject": "^4.0.4",
"@rollup/plugin-json": "^4.1.0",
"@rollup/plugin-node-resolve": "^13.2.1",
Expand Down
20 changes: 7 additions & 13 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 7 additions & 3 deletions src/utils/get-rollup-configs.ts
Expand Up @@ -26,6 +26,8 @@ type EnvObject = {
[key: string]: string;
};

const stripQuery = (url: string) => url.split('?')[0];

const getConfig = {
async type(
options: Options,
Expand Down Expand Up @@ -85,7 +87,9 @@ const getConfig = {
})]
: []
),
commonjs(),
commonjs({
strictRequires: true,
}),
json(),
esbuildTransform(esbuildConfig),
createRequire(),
Expand Down Expand Up @@ -154,7 +158,7 @@ export async function getRollupConfigs(
* - test tmpdir is a symlink: /var/ -> /private/var/
*/
entryFileNames: chunk => (
fs.realpathSync.native(chunk.facadeModuleId!)
fs.realpathSync.native(stripQuery(chunk.facadeModuleId!))
.slice(sourceDirectoryPath.length)
.replace(/\.\w+$/, extension)
),
Expand Down Expand Up @@ -197,7 +201,7 @@ export async function getRollupConfigs(
* - test tmpdir is a symlink: /var/ -> /private/var/
*/
entryFileNames: chunk => (
fs.realpathSync.native(chunk.facadeModuleId!)
fs.realpathSync.native(stripQuery(chunk.facadeModuleId!))
.slice(sourceDirectoryPath.length)
.replace(/\.\w+$/, extension)
),
Expand Down
2 changes: 2 additions & 0 deletions tests/fixture-package/src/cjs.cjs
@@ -1,3 +1,5 @@
console.log('side effect');

module.exports = function sayHello(name) {
console.log('Hello', name);
};
2 changes: 1 addition & 1 deletion tests/fixture-package/src/conditional-require.js
@@ -1,6 +1,6 @@
if (process.env.NODE_ENV === 'production') {
console.log('production');
require('./target');
require('./cjs.cjs');
} else {
console.log('development');
}
Expand Down
20 changes: 19 additions & 1 deletion tests/specs/builds/output-module.ts
Expand Up @@ -72,7 +72,7 @@ export default testSuite(({ describe }, nodePath: string) => {
expect(pkgrollProcess.stderr).toBe('');

const content = await fixture.readFile('dist/cjs.mjs', 'utf8');
expect(content).toMatch('export { cjs as default }');
expect(content).toMatch('export { cjsExports as default }');

await fixture.cleanup();
});
Expand All @@ -98,5 +98,23 @@ export default testSuite(({ describe }, nodePath: string) => {

await fixture.cleanup();
});

test('conditional require() no side-effects', async () => {
const fixture = await createFixture('./tests/fixture-package');

await fixture.writeJson('package.json', {
main: './dist/conditional-require.mjs',
});

const pkgrollProcess = await pkgroll([], { cwd: fixture.path, nodePath });

expect(pkgrollProcess.exitCode).toBe(0);
expect(pkgrollProcess.stderr).toBe('');

const content = await fixture.readFile('dist/conditional-require.mjs', 'utf8');
expect(content).toMatch('\tconsole.log(\'side effect\');');

await fixture.cleanup();
});
});
});

0 comments on commit b9c936c

Please sign in to comment.