Skip to content
This repository was archived by the owner on Apr 4, 2025. It is now read-only.

Move purify transforms#81

Merged
hansl merged 4 commits into
angular:masterfrom
filipesilva:move-purify-transforms
Aug 21, 2017
Merged

Move purify transforms#81
hansl merged 4 commits into
angular:masterfrom
filipesilva:move-purify-transforms

Conversation

@filipesilva
Copy link
Copy Markdown
Contributor

No description provided.

@filipesilva filipesilva requested a review from hansl August 14, 2017 14:17
@filipesilva filipesilva changed the title [WIP] Move purify transforms Move purify transforms Aug 14, 2017
@filipesilva
Copy link
Copy Markdown
Contributor Author

/cc @IgorMinar

Copy link
Copy Markdown
Contributor

@hansl hansl left a comment

Choose a reason for hiding this comment

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

A first round.



export const wrapEnumsRegexes = [
// tslint:disable:max-line-length
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seriously consider making those regexes as parser functions. There are many backreferences and those are really slow. I wouldn't be surprised if this is O(n^4), considering you're going to 4 backrefs. Considering you run this on really long strings I think you'd see some improvements. We can chat about it later.



export const prefixClassRegexes = [
// tslint:disable-next-line:max-line-length
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See my comment above.

// expressions: `(function() {}())` Their start pos doesn't include the opening paren
// and must be adjusted.
if (isIIFE(node)
if (
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

First line goes with the if, last parenthesis with the last line, with 4 spaces of indentation:

if (isIIFE(node)
    && previousNode.kind === ts.SyntaxKind.ParenthesizedExpression
    && node.parent
    && !hasPureComment(node.parent)) {
  topLEvelFunctions.push(node.parent);
} else if ((node.kind === ts.SyntaxKind.CallExpression || node.kind === ts.SyntaxKind.NewExpression)
    && !hasPureComment(node)) {
  topLevelFunctions.push(node);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, but the else if still has to be split because of line length.



export const scrubFileRegexes = [
/decorators/,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

indexOf() is 10x faster than regexes. For static strings like those I'd use indexOf. Reference: https://jsperf.com/regex-vs-strings

enumStatements.push(...findTs2_3EnumStatements(maybeName, nextStatement));
enumDropNodes.push(nextStatement);
} else if (
varDecl.initializer
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See my comment about if and alignment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

const exprStmt = innerStmt as ts.ExpressionStatement;

if (!(
innerBinExpr.operatorToken.kind === ts.SyntaxKind.FirstAssignment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Alignment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

const leftExpr = binExpr.left as ts.PropertyAccessExpression | ts.ElementAccessExpression;

if (!(
leftExpr.expression.kind === ts.SyntaxKind.Identifier
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Alignment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

return ts.visitEachChild(node, visitor, context);
};

return ts.visitNode(sf, visitor);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ts.visitEachChild would be slightly more efficient here. The source file node can't be a down leveled class so no need to check it.

const enums = findEnumDeclarations(sf);
const dropNodes = enums.reduce((acc: ts.Node[], curr) => acc.concat(curr.dropNodes), []);

const visitor: ts.Visitor = (node: ts.Node): ts.Node => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Return type of ts.Node | undefined to remove the any usage below?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

TypeScript has wrong typing here (ts.Visitor), so we can't easily :(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yup, noticed that. The return type of ts.Visitor (ts.VisitResult<Node>) is the issue. It should be defined as T | T[] | undefined.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did a PR for that, should be in one of the newer versions microsoft/TypeScript#17044

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Awesome

hansl
hansl previously approved these changes Aug 21, 2017
@hansl
Copy link
Copy Markdown
Contributor

hansl commented Aug 21, 2017

Please rebase. This looks good to me, just want to run the latest lint and tests.

@hansl hansl merged commit 90383e8 into angular:master Aug 21, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants