Skip to content

Use regex+getTokenAtPosition to find dynamic import #28104

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 24, 2018
Merged

Use regex+getTokenAtPosition to find dynamic import #28104

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 30 additions & 20 deletions src/compiler/program.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1895,12 +1895,9 @@ namespace ts {

for (const node of file.statements) {
collectModuleReferences(node, /*inAmbientModule*/ false);
if ((file.flags & NodeFlags.PossiblyContainsDynamicImport) || isJavaScriptFile) {
collectDynamicImportOrRequireCalls(node);
}
}
if ((file.flags & NodeFlags.PossiblyContainsDynamicImport) || isJavaScriptFile) {
collectDynamicImportOrRequireCalls(file.endOfFileToken);
collectDynamicImportOrRequireCalls(file);
}

file.imports = imports || emptyArray;
Expand Down Expand Up @@ -1952,25 +1949,38 @@ namespace ts {
}
}

function collectDynamicImportOrRequireCalls(node: Node): void {
if (isRequireCall(node, /*checkArgumentIsStringLiteralLike*/ true)) {
imports = append(imports, node.arguments[0]);
}
// we have to check the argument list has length of 1. We will still have to process these even though we have parsing error.
else if (isImportCall(node) && node.arguments.length === 1 && isStringLiteralLike(node.arguments[0])) {
imports = append(imports, node.arguments[0] as StringLiteralLike);
}
else if (isLiteralImportTypeNode(node)) {
imports = append(imports, node.argument.literal);
}
collectDynamicImportOrRequireCallsForEachChild(node);
if (hasJSDocNodes(node)) {
forEach(node.jsDoc, collectDynamicImportOrRequireCallsForEachChild);
function collectDynamicImportOrRequireCalls(file: SourceFile) {
const r = /import|require/g;
Copy link
Contributor

@ajafff ajafff Oct 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

you could tweak the regex to avoid false positives. though you need to adjust the loop a bit to save the result of r.exec to a variable.

/\b(?:import|require)\s*[(/<]/g

The above will not match require.resolve for example as it requires an opening parenthesis or a slash (typically the start of a comment) after it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we'd save much in the majority of files, and I prefer to have the simplest possible regex.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this PR is about performance, I'd rather we are careful to not regress performance where possible.
There is require.resolve, import.meta, in a project of mine there's a lot requiresTypeInformation and in addition these are regular verbs that could occur in a lot of comments.
Each time you encounter something that contains one of these words, you need to recursively look up the node in the AST. That can potentially be very slow for large files with deep nesting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, the new collector is m log(n) where m is the number of import|require and n is the number of nodes. The old collector is n log(n), and I think m will only approach n in pathological cases.

Also, the compiler is going to crash on very deep ASTs in the binder anyway. I tried to fix that by adding a flat OperatorListExpression to replace deep BinaryExpression trees, but gave up (for now) because of the amount of work needed to update all our BinaryExpression-based code.

while (r.exec(file.text) !== null) {
const node = getTokenAtPosition(file, r.lastIndex);
if (isRequireCall(node, /*checkArgumentIsStringLiteralLike*/ true)) {
imports = append(imports, node.arguments[0]);
}
// we have to check the argument list has length of 1. We will still have to process these even though we have parsing error.
else if (isImportCall(node) && node.arguments.length === 1 && isStringLiteralLike(node.arguments[0])) {
imports = append(imports, node.arguments[0] as StringLiteralLike);
}
else if (isLiteralImportTypeNode(node)) {
imports = append(imports, node.argument.literal);
}
}
}

function collectDynamicImportOrRequireCallsForEachChild(node: Node) {
forEachChild(node, collectDynamicImportOrRequireCalls);
/** Returns a token if position is in [start-of-leading-trivia, end) */
function getTokenAtPosition(sourceFile: SourceFile, position: number): Node {
Copy link
Contributor

Choose a reason for hiding this comment

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

does this differ from getTokenAtPosition in services/utitilities.ts? if not, it could be moved to compiler/utilities.ts and also used for services

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it uses Node.pos and Node.end instead of the much-more-complicated Node.getStart() and Node.getFullStart() that are present in the language service. The original getTokenAtPosition handles a lot more cases as well; I'd rather wait until something else in the compiler proper needs this code to move it into utilities and harden it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. In addition it doesn't use node.getChildren to get the actual token. So at least it should be renamed to getNodeAtPosition to avoid confusion.

let current: Node = sourceFile;
const getContainingChild = (child: Node) => {
if (child.pos <= position && (position < child.end || (position === child.end && (child.kind === SyntaxKind.EndOfFileToken)))) {
return child;
}
};
while (true) {
const child = hasJSDocNodes(current) && forEach(current.jsDoc, getContainingChild) || forEachChild(current, getContainingChild);
Copy link
Contributor

Choose a reason for hiding this comment

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

probably unrelated to this change: IMO it should only look at JSDoc in JS files as ImportTypeNodes in JSDoc of TS files have no effect.

if (!child) {
return current;
}
current = child;
}
}
}

Expand Down