-
Notifications
You must be signed in to change notification settings - Fork 25
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
Preserve directive prologues fixes #5 #7 #6
Conversation
The most important goal is correctness. Directives were destroyed by placing imports above it. Directive prologues are defined by the spec as > A Directive Prologue is the longest sequence of ExpressionStatements occurring as > the initial StatementListItems or ModuleItems of a FunctionBody, a ScriptBody, or > a ModuleBody and where each ExpressionStatement in the sequence consists entirely > of a StringLiteral token followed by a semicolon. The semicolon may appear explicitly > or may be inserted by automatic semicolon insertion (12.9). A Directive Prologue may > be an empty sequence.
Previously, a regex string replacement was used, and regex is an inadequate tool for transforming entire JavaScript code files. Instead, this commit changes that to a proper algorithm that removes the code at the source position ranges of the nodes to be removed. This will not work properly if the Babel parser ever did not return the correct source positions, but that is not a regression since the former algortihm already relied on the positions being correct.
The comments are added back as part of the new directives placed at the top, so we should remove them from the original code we add after the directives and the imports.
I can't give this a thorough review right now, but I wonder, is there a way to check if there is a performance impact to this change? |
We could of course do it manually, not sure if we can add performance tests. But regarding how it "should" affect the performance:
So I wouldn't expect this to have an effect on performance (if anything it could improve it due to less Regexes). But perhaps we should take a large JS file and compare the times it takes to format, just so we can rule out an unexpected bottleneck. |
@IanVS Do you have time to take a look at this in the next few days? Otherwise do you mind if I do a quick test myself and then merge? |
I'll give it a quick glance through today and approve. Curious about perf, but I don't think that's a blocker. |
@blutorange In reading the spec, I see that it says:
I'm not so familiar with the spec, is an empty sequence different from an empty statement? I guess an empty sequence is |
@IanVS As far as I understand it
An empty sequence is like the empty set, so that's just their way of saying that having the directive prologue is optional. This is a source file with an empty directive prologue list, consisting of an empty statement and a function declaration statement. ;
function foo() {} This is a source file with a single-element directive sequence, and the value of the directive is the empty string. "";
function foo() {} This is also source file with a single-element directive sequence. (The semicolon is inserted via the process of automatic semicolon insertion) ""
function foo() {} It doesn't really matter though, since we aren't doing the parsing ourselves and just rely on the parser being implemented correctly. I just added some tests for these cases just in case somebody gets the idea to implement the parsing manually ; ) PS: You can use e.g. https://astexplorer.net/ to see how a piece of code parses. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this out in my own project, and it seems to make no difference in performance, prettier takes roughly 14.5 seconds to check the formatting either way, and I did not get any formatting problems when running this branch. Nicely done!
* @param directives All directive prologues from the original code (e.g. | ||
* `"use strict";`). | ||
* @param interpreter Optional interpreter directives, if present (e.g. | ||
* `#!/bin/node`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These comments are helpful, thanks.
// | ||
// |-----------xxxxx-----xxxx-----xxxx-----------| | ||
// ^---------^ | ||
// This part |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I've come to the realization that otherwise even I myself won't understand what's going on when I look at code weeks or months later ; )
This fixes #5 and also includes the fix for #7 since I need it for this. While the relevance of strict directives may be doubtful for modules, correctness is still a goal and we should still preserve the original code and only order imports.
For references, this code
now becomes
and the following
still becomes
since an empty statement is not a directive prologue according to the spec.