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

Refactor tree-shaking rendering #1949

Merged
merged 27 commits into from
Feb 15, 2018
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
4b9dfc3
Make BlockStatements take care of removing their children
lukastaegert Jan 28, 2018
2436ee1
Extract BlockStatement rendering to be used for Program nodes with
lukastaegert Jan 28, 2018
48f4683
Use new logic for switch statements (which works much better than the
lukastaegert Jan 28, 2018
152eb29
* Properly handle multi-line comments after statements
lukastaegert Feb 2, 2018
eae7d96
Refactor and simplify default export rendering
lukastaegert Feb 2, 2018
c345fb5
Clean up code, improve performance and resolve #1850
lukastaegert Feb 2, 2018
7781b5f
Resolve #1772
lukastaegert Feb 2, 2018
698f3a2
Remove "statement" concept and other remains of old positioning logic
lukastaegert Feb 2, 2018
a395faf
When checking for symbols other than line-breaks, check for both kinds
lukastaegert Feb 3, 2018
de9acd0
First draft for variable declarations
lukastaegert Feb 6, 2018
ed77a00
* Simplify declaration rendering logic
lukastaegert Feb 6, 2018
d2720f6
Prevent trailing white-space
lukastaegert Feb 6, 2018
6bb3829
Remove line-breaks after declaration key-word
lukastaegert Feb 6, 2018
2a570a2
Resolve #1937
lukastaegert Feb 6, 2018
3588a4a
Resolve #1831
lukastaegert Feb 6, 2018
9ff220f
Resolve #1943
lukastaegert Feb 7, 2018
6b30972
Include line-breaks with the previous statement instead of the
lukastaegert Feb 8, 2018
103e16f
Rework declaration handling to associate comments after a comma but
lukastaegert Feb 11, 2018
377fe90
Fix TypeScript test
lukastaegert Feb 11, 2018
495f97a
* Improve some names
lukastaegert Feb 12, 2018
acaea3d
* Reintroduce statement concept as an alias
lukastaegert Feb 12, 2018
d45a74c
Reduce number of necessary slices by supplying start indices
lukastaegert Feb 13, 2018
adeebc2
Only calculate boundaries if necessary
lukastaegert Feb 13, 2018
8ecadf2
Store flags for AST nodes directly on the prototype
lukastaegert Feb 13, 2018
114149f
Improve variable declaration performance
lukastaegert Feb 14, 2018
22aa1fb
* Use charCodeAt where possible to improve performance
lukastaegert Feb 14, 2018
bb8b81d
Slightly clean up source map removal
lukastaegert Feb 15, 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
2 changes: 1 addition & 1 deletion src/Chunk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -517,7 +517,7 @@ export default class Chunk {
forOwn(module.scope.variables, variable => {
if (!(<ExportDefaultVariable>variable).isDefault || !(<ExportDefaultVariable>variable).hasId) {
let safeName;
if (es || system || !variable.isReassigned) {
if (es || system || !variable.isReassigned || variable.isId) {
safeName = getSafeName(variable.name);
} else {
const safeExportName = this.exportedVariables.get(variable);
Expand Down
32 changes: 15 additions & 17 deletions src/Module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ import { isTemplateLiteral } from './ast/nodes/TemplateLiteral';
import { isLiteral } from './ast/nodes/Literal';
import Chunk, { DynamicImportMechanism } from './Chunk';

export interface IdMap { [key: string]: string; }
export interface IdMap {[key: string]: string;}

export interface CommentDescription {
block: boolean;
Expand Down Expand Up @@ -107,7 +107,15 @@ export interface RenderOptions {
freeze: boolean;
importMechanism?: DynamicImportMechanism;
systemBindings: boolean;
};
}

export interface NodeRenderOptions {
start?: number,
end?: number,
isNoStatement?: boolean
}

export const NO_SEMICOLON: NodeRenderOptions = { isNoStatement: true };

export default class Module {
type: 'Module';
Expand Down Expand Up @@ -251,7 +259,7 @@ export default class Module {

// export { name } from './other'
if (source) {
if (!~this.sources.indexOf(source)) this.sources.push(source);
if (this.sources.indexOf(source) === -1) this.sources.push(source);

if (node.type === NodeType.ExportAllDeclaration) {
// Store `export * from '...'` statements in an array of delegates.
Expand Down Expand Up @@ -343,7 +351,7 @@ export default class Module {
private addImport (node: ImportDeclaration) {
const source = node.source.value;

if (!~this.sources.indexOf(source)) this.sources.push(source);
if (this.sources.indexOf(source) === -1) this.sources.push(source);

node.specifiers.forEach(specifier => {
const localName = specifier.local.name;
Expand All @@ -369,20 +377,13 @@ export default class Module {
}

private analyse () {
enhance(this.ast, this, this.comments, this.dynamicImports);

// discover this module's imports and exports
let lastNode: Node;

enhance(this.ast, this, this.dynamicImports);
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're no longer using this.comments does that mean this property can be removed entirely, including the use of the onComment hook?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just tested this and the answer is yes :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately not as it is still required to remove source map comments. This, however, is the only reason why we keep this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh. Can you point me to the code?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe at some point we should implement our own algorithm to scan for and remove those comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see. A sourceMappingURL comment can only be a block comment so it's fine to do something custom. Also it can be based on lastIndexOf as a backwards search through the string would be quicker.

Here's the code I have used for this previously - https://github.com/systemjs/builder/blob/master/lib/sourcemaps.js#L12.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at this example, maybe the issue is not that trivial and we should probably rather trust acorns parsing here and leave it as it is:

Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise we might break the sourceMap handling of other libraries.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's v8's method to do this - https://github.com/v8/v8/blob/8d38c15e04598f9e48e6230327da0e41a2445957/src/inspector/search-util.cc#L16

Note that it doesn't use a lexer at all.

I think it would be much simpler to make this code its own method and deprecate this.comments entirely. But happy to make this a separate PR too if you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about this further detection is a less stringent problem than removal - and that’s what I got confused here. It makes sense that Rollup has to hold to a higher standard for removal not to break code.

Perhaps we can inline the removal into the parsing code without separately gathering the comments at least.

this.ast.body.forEach(node => {
if ((<ImportDeclaration>node).isImportDeclaration) {
this.addImport(<ImportDeclaration>node);
} else if ((<ExportDefaultDeclaration | ExportNamedDeclaration | ExportAllDeclaration>node).isExportDeclaration) {
this.addExport((<ExportDefaultDeclaration | ExportNamedDeclaration | ExportAllDeclaration>node));
}

if (lastNode) lastNode.next = node.leadingCommentStart || node.start;
lastNode = node;
});
}

Expand Down Expand Up @@ -588,10 +589,7 @@ export default class Module {

render (options: RenderOptions): MagicString {
const magicString = this.magicString.clone();

this.ast.body.forEach(node => {
node.render(magicString, options);
});
this.ast.render(magicString, options);

if (this.namespace().needsNamespaceBlock) {
magicString.append(
Expand Down Expand Up @@ -648,7 +646,7 @@ export default class Module {
// namespace
if (name.length === 1) {
return this.namespace();
// export * from 'external'
// export * from 'external'
} else {
const module = <ExternalModule>this.graph.moduleById.get(name.slice(1));
return module.traceExport('*');
Expand Down
4 changes: 0 additions & 4 deletions src/ast/comment.ts

This file was deleted.

20 changes: 1 addition & 19 deletions src/ast/enhance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,31 +3,13 @@ import UnknownNode from './nodes/UnknownNode';
import keys from './keys';
import { Node } from './nodes/shared/Node';
import Module from '../Module';
import Comment from './comment';
import MagicString from 'magic-string';
import Import from './nodes/Import';

const newline = /\n/;

export default function enhance (ast: any, module: Module, comments: Comment[], dynamicImportReturnList: Import[]) {
export default function enhance (ast: any, module: Module, dynamicImportReturnList: Import[]) {
enhanceNode(ast, {}, module, module.magicString, dynamicImportReturnList);

let comment = comments.shift();

for (const node of ast.body) {
if (comment && comment.start < node.start) {
node.leadingCommentStart = comment.start;
}

while (comment && comment.end < node.end) comment = comments.shift();

// if the next comment is on the same line as the end of the node,
// treat is as a trailing comment
if (comment && !newline.test(module.code.slice(node.end, comment.start))) {
node.trailingCommentEnd = comment.end; // TODO is node.trailingCommentEnd used anywhere?
comment = comments.shift();
}

node.initialise(module.scope);
}
}
Expand Down
12 changes: 3 additions & 9 deletions src/ast/nodes/BlockStatement.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@ import { UNKNOWN_EXPRESSION } from '../values';
import ExecutionPathOptions from '../ExecutionPathOptions';
import Scope from '../scopes/Scope';
import MagicString from 'magic-string';
import { Node } from './shared/Node';
import { StatementBase, StatementNode } from './shared/Statement';
import { Node, StatementBase, StatementNode } from './shared/Node';
import { NodeType } from './NodeType';
import { RenderOptions } from '../../Module';
import { renderStatementList } from '../../utils/renderHelpers';

export function isBlockStatement (node: Node): node is BlockStatement {
return node.type === NodeType.BlockStatement;
Expand Down Expand Up @@ -48,12 +48,8 @@ export default class BlockStatement extends StatementBase {
}

initialiseChildren (_parentScope: Scope) {
let lastNode;
for (const node of this.body) {
node.initialise(this.scope);

if (lastNode) lastNode.next = node.start;
lastNode = node;
}
}

Expand All @@ -63,9 +59,7 @@ export default class BlockStatement extends StatementBase {

render (code: MagicString, options: RenderOptions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should make start and end mandatory arguments for all render calls, defaulting to start = this.start?

if (this.body.length) {
for (const node of this.body) {
node.render(code, options);
}
renderStatementList(this.body, code, this.start + 1, this.end - 1, options);
} else {
super.render(code, options);
}
Expand Down
2 changes: 1 addition & 1 deletion src/ast/nodes/BreakStatement.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import ExecutionPathOptions from '../ExecutionPathOptions';
import Identifier from './Identifier';
import { StatementBase } from './shared/Statement';
import { NodeType } from './NodeType';
import { StatementBase } from './shared/Node';

export default class BreakStatement extends StatementBase {
type: NodeType.BreakStatement;
Expand Down
23 changes: 12 additions & 11 deletions src/ast/nodes/ClassDeclaration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,28 +4,29 @@ import Identifier from './Identifier';
import MagicString from 'magic-string';
import { NodeType } from './NodeType';
import { RenderOptions } from '../../Module';
import { Node } from './shared/Node';

export function isClassDeclaration (node: Node): node is ClassDeclaration {
return node.type === NodeType.ClassDeclaration;
}

export default class ClassDeclaration extends ClassNode {
type: NodeType.ClassDeclaration;
id: Identifier;

initialiseChildren (parentScope: Scope) {
// Class declarations are like let declarations: Not hoisted, can be reassigned, cannot be redeclared
this.id && this.id.initialiseAndDeclare(parentScope, 'class', this);
if (this.id) {
this.id.initialiseAndDeclare(parentScope, 'class', this);
this.id.variable.isId = true;
}
super.initialiseChildren(parentScope);
}

render (code: MagicString, options: RenderOptions) {
if (!this.module.graph.treeshake || this.included) {
if (options.systemBindings && this.id.variable.exportName) {
code.appendRight(this.end, ` exports('${this.id.variable.exportName}', ${this.id.variable.getName()});`);
}
super.render(code, options);
} else {
code.remove(
this.leadingCommentStart || this.start,
this.next || this.end
);
if (options.systemBindings && this.id.variable.exportName) {
code.appendRight(this.end, ` exports('${this.id.variable.exportName}', ${this.id.variable.getName()});`);
}
super.render(code, options);
}
}
3 changes: 1 addition & 2 deletions src/ast/nodes/DoWhileStatement.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import ExecutionPathOptions from '../ExecutionPathOptions';
import { StatementBase, StatementNode } from './shared/Statement';
import { ExpressionNode } from './shared/Node';
import { ExpressionNode, StatementNode, StatementBase } from './shared/Node';
import { NodeType } from './NodeType';

export default class DoWhileStatement extends StatementBase {
Expand Down
2 changes: 1 addition & 1 deletion src/ast/nodes/EmptyStatement.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import MagicString from 'magic-string';
import { StatementBase } from './shared/Statement';
import { NodeType } from './NodeType';
import { RenderOptions } from '../../Module';
import { StatementBase } from './shared/Node';

export default class EmptyStatement extends StatementBase {
type: NodeType.EmptyStatement;
Expand Down
15 changes: 8 additions & 7 deletions src/ast/nodes/ExportAllDeclaration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,19 @@ import { NodeBase } from './shared/Node';
import Literal from './Literal';
import MagicString from 'magic-string';
import { NodeType } from './NodeType';
import { RenderOptions } from '../../Module';
import { NodeRenderOptions, RenderOptions } from '../../Module';
import { BLANK } from '../../utils/object';

export default class ExportAllDeclaration extends NodeBase {
type: NodeType.ExportAllDeclaration;
source: Literal<string>;
isExportDeclaration: true;
needsBoundaries: true;

initialiseNode () {
this.isExportDeclaration = true;
}

render (code: MagicString, _options: RenderOptions) {
code.remove(this.leadingCommentStart || this.start, this.next || this.end);
render (code: MagicString, _options: RenderOptions, { start, end }: NodeRenderOptions = BLANK) {
code.remove(start, end);
}
}

ExportAllDeclaration.prototype.needsBoundaries = true;
ExportAllDeclaration.prototype.isExportDeclaration = true;