Skip to content
This repository has been archived by the owner on Aug 4, 2021. It is now read-only.

Prepare for 1.0.0, fix some issues #349

Merged
merged 4 commits into from
Oct 10, 2018
Merged

Prepare for 1.0.0, fix some issues #349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
32ea46c
Fix warning when importing ESM from CJS and optimize generated code
lukastaegert Oct 4, 2018
5a219b2
Make tests compatible with rollup 1.0
lukastaegert Oct 5, 2018
ad87764
Resolve #348
lukastaegert Oct 5, 2018
b2e1fa9
Resolve #341
lukastaegert Oct 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,13 @@ export function commonjsRequire () {
}

export function unwrapExports (x) {
return x && x.__esModule && Object.prototype.hasOwnProperty.call(x, 'default') ? x['default'] : x;
return x && x.__esModule && Object.prototype.hasOwnProperty.call(x, 'default') ? x.default : x;
}

export function createCommonjsModule(fn, module) {
return module = { exports: {} }, fn(module, module.exports), module.exports;
}

export function getCjsExportFromNamespace (n) {
return n && n.default || n;
}`;
54 changes: 28 additions & 26 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import {extname, resolve} from 'path';
import {sync as nodeResolveSync} from 'resolve';
import {createFilter} from 'rollup-pluginutils';
import {EXTERNAL_PREFIX, HELPERS, HELPERS_ID, PROXY_PREFIX} from './helpers.js';
import {getIsCjsPromise, setIsCjsPromise} from './is-cjs';
import {getResolveId} from './resolve-id';
import {checkEsModule, hasCjsKeywords, transformCommonjs} from './transform.js';
import {getName} from './utils.js';
import { extname, resolve } from 'path';
import { sync as nodeResolveSync } from 'resolve';
import { createFilter } from 'rollup-pluginutils';
import { EXTERNAL_PREFIX, HELPERS, HELPERS_ID, PROXY_PREFIX } from './helpers.js';
import { getIsCjsPromise, setIsCjsPromise } from './is-cjs';
import { getResolveId } from './resolve-id';
import { checkEsModule, hasCjsKeywords, transformCommonjs } from './transform.js';
import { getName } from './utils.js';

export default function commonjs(options = {}) {
const extensions = options.extensions || ['.js'];
Expand All @@ -27,7 +27,8 @@ export default function commonjs(options = {}) {
});
}

const esModulesWithoutDefaultExport = [];
const esModulesWithoutDefaultExport = Object.create(null);
const esModulesWithDefaultExport = Object.create(null);
const allowDynamicRequire = !!options.ignore; // TODO maybe this should be configurable?

const ignoreRequire =
Expand Down Expand Up @@ -74,40 +75,42 @@ export default function commonjs(options = {}) {
const actualId = id.slice(PROXY_PREFIX.length);
const name = getName(actualId);

return (extensions.indexOf(extname(id)) === -1
? Promise.resolve(false)
: getIsCjsPromise(actualId)
).then(isCjs => {
return getIsCjsPromise(actualId).then(isCjs => {
if (isCjs)
return `import { __moduleExports } from ${JSON.stringify(
actualId
)}; export default __moduleExports;`;
else if (esModulesWithoutDefaultExport.indexOf(actualId) !== -1)
else if (esModulesWithoutDefaultExport[actualId])
return `import * as ${name} from ${JSON.stringify(actualId)}; export default ${name};`;
else if (esModulesWithDefaultExport[actualId]) {
return `export {default} from ${JSON.stringify(actualId)};`;
}
else
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the logic for modules that neither don't have a default nor have a default?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They get a deoptimized wrapper. This is the case for

  • modules that are ignored because of some include/exclude pattern
  • modules that are ignored because they do not have one of the allowed extensions

The wrapper looks like this:

// before bundling
import * as foo from './foo.js';
export default getCjsExportFromNamespace(foo);

function getCjsExportFromNamespace(n) {
  // as the property access is in a function, Rollup does not try to optimize `foo.default`
  // which could result in a "default is not exported" warning
  return n && n.default || n;
}

// after bundling
// the namespace from "foo.js"
var foo = /*#__PURE__*/Object.freeze({
  namedExport: ...,
  default: ...
});

function getCjsExportFromNamespace (n) {
  return n && n.default || n;
}

// this is what is returned by require("./foo")
var require$$0 = getCjsExportFromNamespace(foo);
...

This is slightly less optimal than the previous version of the plugin which directly inserted the foo && foo.default || foo without another function call. As noted in the comment above, however, this produced warnings in recent rollup versions. I assume the reason why previously, the default was accessed via foo['default'] was that some old version of rollup did not optimize this.
As the getCjsExportFromNamespace is now shared between all wrappers, it is unlikely to be optimized by rollup in the future.

I am not 100% happy about the n && n.default || n. This was the previous code. My feeling is that either

n && Object.prototype.hasOwnProperty.call(n, 'default') ? n.default : n

or

n && ('default' in n) ? n.default : n

would produce better results, but I was not sure about all implications.

return `import * as ${name} from ${JSON.stringify(
actualId
)}; export default ( ${name} && ${name}['default'] ) || ${name};`;
)}; import {getCjsExportFromNamespace} from "${HELPERS_ID}"; export default getCjsExportFromNamespace(${name})`;
});
}
},

transform(code, id) {
if (!filter(id)) return null;
if (extensions.indexOf(extname(id)) === -1) return null;
if (!filter(id) || extensions.indexOf(extname(id)) === -1) {
setIsCjsPromise(id, Promise.resolve(null));
return null;
}

const transformPromise = entryModuleIdsPromise
.then(entryModuleIds => {
const { isEsModule, hasDefaultExport, ast } = checkEsModule(this.parse, code, id);
if (isEsModule) {
if (!hasDefaultExport) esModulesWithoutDefaultExport.push(id);
return;
(hasDefaultExport ? esModulesWithDefaultExport : esModulesWithoutDefaultExport)[id] = true;
return null;
}

// it is not an ES module but not a commonjs module, too.
// it is not an ES module but it does not have CJS-specific elements.
if (!hasCjsKeywords(code, ignoreGlobal)) {
esModulesWithoutDefaultExport.push(id);
return;
esModulesWithoutDefaultExport[id] = true;
return null;
}

const transformed = transformCommonjs(
Expand All @@ -123,8 +126,8 @@ export default function commonjs(options = {}) {
ast
);
if (!transformed) {
esModulesWithoutDefaultExport.push(id);
return;
esModulesWithoutDefaultExport[id] = true;
return null;
}

return transformed;
Expand All @@ -133,8 +136,7 @@ export default function commonjs(options = {}) {
this.error(err, err.loc);
});

setIsCjsPromise(id, transformPromise.then(Boolean, () => true));

setIsCjsPromise(id, transformPromise.then(Boolean, () => false));
return transformPromise;
}
};
Expand Down
26 changes: 16 additions & 10 deletions src/transform.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import {walk} from 'estree-walker';
import { walk } from 'estree-walker';
import MagicString from 'magic-string';
import {attachScopes, makeLegalIdentifier} from 'rollup-pluginutils';
import {extractNames, flatten, isFalsy, isReference, isTruthy} from './ast-utils.js';
import {HELPERS_ID, PROXY_PREFIX} from './helpers.js';
import {getName} from './utils.js';
import { attachScopes, makeLegalIdentifier } from 'rollup-pluginutils';
import { extractNames, flatten, isFalsy, isReference, isTruthy } from './ast-utils.js';
import { HELPERS_ID, PROXY_PREFIX } from './helpers.js';
import { getName } from './utils.js';

const reserved = 'process location abstract arguments boolean break byte case catch char class const continue debugger default delete do double else enum eval export extends false final finally float for from function goto if implements import in instanceof int interface let long native new null package private protected public return short static super switch synchronized this throw throws transient true try typeof var void volatile while with yield'.split(
' '
Expand Down Expand Up @@ -46,15 +46,21 @@ export function hasCjsKeywords(code, ignoreGlobal) {
export function checkEsModule(parse, code, id) {
const ast = tryParse(parse, code, id);

// if there are top-level import/export declarations, this is ES not CommonJS
let hasDefaultExport = false;
let isEsModule = false;
for (const node of ast.body) {
if (node.type === 'ExportDefaultDeclaration') hasDefaultExport = true;
if (importExportDeclaration.test(node.type)) isEsModule = true;
if (node.type === 'ExportDefaultDeclaration')
return { isEsModule: true, hasDefaultExport: true, ast };
if (node.type === 'ExportNamedDeclaration') {
isEsModule = true;
for (const specifier of node.specifiers) {
if (specifier.exported.name === 'default') {
return { isEsModule: true, hasDefaultExport: true, ast };
}
}
} else if (importExportDeclaration.test(node.type)) isEsModule = true;
}

return { isEsModule, hasDefaultExport, ast };
return { isEsModule, hasDefaultExport: false, ast };
}

export function transformCommonjs(
Expand Down
1 change: 1 addition & 0 deletions test/function/export-default-from/_config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module.exports = {};
1 change: 1 addition & 0 deletions test/function/export-default-from/imported.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default 'default export';
1 change: 1 addition & 0 deletions test/function/export-default-from/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
assert.equal(require('./reexporter'), 'default export');
1 change: 1 addition & 0 deletions test/function/export-default-from/reexporter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export {default} from './imported';
15 changes: 15 additions & 0 deletions test/function/resolve-is-cjs-extension/_config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
module.exports = {
description: 'always resolve cjs detection even if an imported file has an unknown extension',
options: {
plugins: [
{
resolveId(importee) {
if (importee === 'second') {
return `${__dirname}/second.x`;
}
}
}
]
},
pluginOptions: {}
};
1 change: 1 addition & 0 deletions test/function/resolve-is-cjs-extension/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
assert.equal(require('second').result, 'second' );
1 change: 1 addition & 0 deletions test/function/resolve-is-cjs-extension/second.x
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const result = 'second';
17 changes: 17 additions & 0 deletions test/function/resolve-is-cjs-filtered/_config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
module.exports = {
description: 'always resolve cjs detection even if an imported file is filtered',
options: {
plugins: [
{
resolveId(importee) {
if (importee === 'second') {
return `${__dirname}/second.js`;
}
}
}
]
},
pluginOptions: {
include: ['function/resolve-is-cjs-filtered/main.js']
}
};
1 change: 1 addition & 0 deletions test/function/resolve-is-cjs-filtered/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
assert.equal(require('second').result, 'second' );
1 change: 1 addition & 0 deletions test/function/resolve-is-cjs-filtered/second.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const result = 'second';