benjamn / recast Goto Github PK

This will make it easier to run the test suite in a browser, among other benefits.

https://github.com/facebook/regenerator does this and it works pretty well: https://github.com/facebook/regenerator/blob/e2696d244b/lib/util.js#L13-L38

This addition will give meaning to the start and end properties of the location object.

The value of the lines property will be a reference to the Lines object that was created by the Parser.

If we add the lines property to the original AST, it will be copied over to the copy returned by parser.getAst(), provided we make the property enumerable. Not sure if this is the behavior we want, though. Maybe make the lines property non-enumerable, to keep it somewhat out of sight?

The subtle benefit of this change is that the Printer will no longer have to know anything about the Parser in order to conservatively reprint a node, and we can move getReprinter into lib/patcher.js.

Padding spaces might work fine when everything is generically printed, but when individual nodes get reprinted, leading/trailing spaces lead to strange extra spacing.

When a property is undefined on Syntax, the case for that type will match any undefined node.type. That seems like an invitation for bizarro clowntown times.

The matching behavior would be much more predictable (and potentially faster) if all the case values were just string literals.

These tests could be organized into directories named after the purpose of the test. For example:

test/
    consoleLogRemoval/
        given.js
        visitor.js
        expected.js

The test/cosoleLogRemoval/visitor.js file should be a module that exports a visitor property that can be used to transform the given code into the expected code. In this case, one might imagine the visitor would remove CallExpressions involving console.log.

The results can be evaluated in two phases. First we should check that the AST of the expected code is equivalent to the AST produced by the visitor. Then the modified AST should be reprinted and the resulting code should be compared against the contents of expected.js.

Comment handling deserves to be tested, especially because comment support is a relatively recent addition to the esprima parser, and having tests would allow us to upgrade esprima with confidence that its comment support is still compatible with our needs.

Using Underscore version 1.4.2:
http://underscorejs.org/underscore.js

Stack trace:

assert.js:102
  throw new assert.AssertionError({
        ^
AssertionError: false == true
    at pushSlice (/Users/benjamn/node_modules/recast/lib/patcher.js:34:20)
    at Patcher.self.get (/Users/benjamn/node_modules/recast/lib/patcher.js:50:9)
    at self.getReprinter (/Users/benjamn/node_modules/recast/lib/parser.js:63:28)
    at print (/Users/benjamn/node_modules/recast/lib/printer.js:42:20)
    at self.getReprinter (/Users/benjamn/node_modules/recast/lib/parser.js:59:21)
    at Array.forEach (native)
    at self.getReprinter (/Users/benjamn/node_modules/recast/lib/parser.js:55:22)
    at print (/Users/benjamn/node_modules/recast/lib/printer.js:42:20)
    at self.getReprinter (/Users/benjamn/node_modules/recast/lib/parser.js:59:21)
    at Array.forEach (native)

Plan: allow an alternate esprima module object to be passed with the options.

Because "JavaScript syntax tree transformer and conservative pretty-printer" doesn't exactly explain everything.

Extend #52 by adding sourceRoot to resulting source map for correct resolving of source URLs by browser.

Support for source maps needs to be baked into the Lines abstraction. Mappings can come from the original, complete Lines object, or from replacements made by the Patcher abstraction. Every Lines object should have a private, immutable set of mappings, and when Lines are sliced and joined those mappings should get merged appropriately, so that the final Lines object has a maximal set of mappings back to the original source.

Right now it's generally assumed in various places to be four.

Indentation could potentially be inferred by the Parser, but not in pathological cases.

Primarily it's the Printer that needs to know what indentation to use, since that's the only code that introduces its own indentation.

Ideally mocha test/lines.js would produce output similar to the "lines" section of the npm test output. This used to work with whiskey, and would be nice if it worked again.

To be clear, I have no problem with sacrificing features like this temporarily to make progress quickly. Switching to mocha was clearly a good move.

Promising model to follow: https://github.com/mishoo/UglifyJS2/blob/master/lib/output.js#L421

MIT, of course.

Now that #43 is (about to be) resolved, let's tell people how source maps work.

For example:

~/node_modules/recast% example/add-braces main.js 

/Users/benjamn/node_modules/recast/main.js:35
    var writeback = options.writeback || defaultWriteback;
                           ^
TypeError: Cannot read property 'writeback' of undefined
    at runString (/Users/benjamn/node_modules/recast/main.js:35:28)
    at /Users/benjamn/node_modules/recast/main.js:26:9
    at fs.js:266:14
    at Object.oncomplete (fs.js:107:15)

https://github.com/jashkenas/docco

Mocha is now the test runner used by npm test (as of #42), but it works by wrapping the old Whiskey syntax in describe blocks. We should migrate the test files to actual Mocha syntax over time.

When we support source maps (#43), it will be convenient to add another property called map to the result object.

This will require a minor version bump, obviously. Other breaking changes to the API should happen at the same time.

To smooth the transition, the result object should probably have a toString method that returns the printed source but writes a warning to process.stderr.

This can be done in parallel, since the processing of one file is unaffected by the processing of any others.

The functional semantics of the processing would have to change, though: instead of simply printing the new contents of the file, the script would need to overwrite the original files with their new contents.

A tool like git add -p could be used to sort through the changes afterward.

This might even be preferable, because not everyone knows how to replace the contents of a file using a tool like sponge or similar.

npm install recast --save-dev

npm http GET https://registry.npmjs.org/ast-types/-/ast-types-0.3.10.tgz
npm http 404 https://registry.npmjs.org/ast-types/-/ast-types-0.3.10.tgz
npm ERR! fetch failed https://registry.npmjs.org/ast-types/-/ast-types-0.3.10.tgz
npm ERR! Error: 404 Not Found
npm ERR!     at WriteStream.<anonymous> (/usr/local/lib/node_modules/npm/lib/utils/fetch.js:57:12)
npm ERR!     at WriteStream.EventEmitter.emit (events.js:117:20)
npm ERR!     at fs.js:1596:14
npm ERR!     at Object.oncomplete (fs.js:107:15)
npm ERR! If you need help, you may report this log at:
npm ERR!     <http://github.com/isaacs/npm/issues>
npm ERR! or email it to:
npm ERR!     <[email protected]>

npm ERR! System Darwin 13.0.0
npm ERR! command "node" "/usr/local/bin/npm" "install" "recast" "--save-dev"
npm ERR! cwd /Users/aylott/Projects/Facebook/react
npm ERR! node -v v0.10.5
npm ERR! npm -v 1.2.18
npm ERR! 
npm ERR! Additional logging details can be found in:
npm ERR!     /Users/aylott/Projects/Facebook/react/npm-debug.log
npm ERR! not ok code 0

Right now, Recast supports generating completely new source maps, assuming the code parsed by recast.parse is the original source code. If you're using Recast for your whole build pipeline, this might be a reasonable assumption, but it's easy to imagine situations where you use another tool earlier in the build process that generates its own source map, and you want to compose that source map with the one produced by Recast to get one complete source map.

Since this is similar to UglifyJS's --in-source-map option, a good interface might be options.inSourceMap or options.inputSourceMap.

I think Lines.prototype.getSourceMap can be left alone, so that it continues to return a new source map, and then Printer.prototype.print can call lines.getSourceMap and compose the result with options.inputSourceMap.

The most annoying and error-prone part of manipulating abstract syntax trees is creating new nodes with appropriate child nodes.

The code in lib/builder.js represents the bare minimum that was needed to write certain kinds of transformations, but ideally it should support the entire Mozilla Parser API:
https://developer.mozilla.org/en-US/docs/SpiderMonkey/Parser_API

The best approach to testing lib/builder.js would be to add strict type assertions everywhere possible and then just be sure to exercise each builder function.

Doing a tree traversal to determine equality should be much cheaper than blindly reprinting.

If you had this, you'd be the best JS beautifier out there. To glory!

At the moment, each tab is converted to four spaces in lib/parser.js, which isn't even really correct except for tabs that begin at offsets that are a multiple of four.

It would be ideal to reuse existing whitespace wherever possible, if only to avoid making superficial changes to parts of the source that were not altered.

Any lines that differ in more than whitespace can be printed however is easiest.

I haven't actually needed to deviate from upstream with my fork of Esprima, and NPM can't always clone the repo from github.com (even though it could install esprima from NPM).

If/when the tests pass with the default version of Esprima installed via npm install esprima, let's just use that.

The appropriate indentation for a node is not necessarily the amount of leading whitespace on the line where the node starts, as this code naïvely assumes:

function getIndent(node) {
    // TODO Improve this.
    return lines.getIndentAt(node.loc.start.line);
}

The correct indentation for a node is something closer to the amount of leading whitespace on the line where the nearest Statement ancestor of the node begins.

This ad-hoc property shouldn't be necessary if we have Path information for the original AST:

> ast = recast.parse('var abc = 1', {sourceFileName: '1.js'})
> recast.print(ast, {sourceMapName: '1.js.map'}).map.names
[ 'v',
  'a',
  'r',
  'b',
  'c',
  '=',
  '1' ]

• Reserved keywords should not appear in names field of source map.
• Real identifier names should not be split into individual characters.

The writeback option only made sense the in context of require("recast").run, which is now deprecated in favor of .parse and .print.

When the Printer falls back to printing a node generically, any comments that fall within the node but after the last child of the node will not be reprinted. Though rare, such comments certainly should be preserved if possible.

This should be a simple matter of associating trailing comments with the previous child in require("comments").add, and making sure to print them in printWithComments (in lib/parser.js).

For instance, we currently assume that the parameters of a function will all be identifiers, which implies that they will not contain line breaks, but we might need to print a comment on the line before a parameter name, or we might eventually support destructuring parameter syntax, which could span multiple lines.

The Lines.prototype.indentTail function will have no effect on Lines objects that are not multi-line (in fact, because Lines objects are immutable, it will just return the original object). However, it needs to be called whenever a multi-line child would need to be indented.

Right now the only indication that something went wrong is STDERR output to the console, which is inconvenient in situations where Recast is being used programmatically.

Currently, if you change the number of statements at the top level of a file, all the whitespace between top-level statements gets reprinted according to the rules of the pretty printer, which isn't the worst thing in the world, but it does generate unnecessary changes.

We should do a better job of surgically replacing old statements with new statements without altering the surrounding whitespace so much, although it would be ideal if completely new statements introduced an appropriate amount of whitespace.

Stack trace:

assert.js:102
  throw new assert.AssertionError({
        ^
AssertionError: "unexpected whitespace character"  "\r"
    at countSpaces (/Users/benjamn/node_modules/recast/lib/lines.js:85:20)
    at /Users/benjamn/node_modules/recast/lib/lines.js:111:21
    at Array.map (native)
    at Object.fromString (/Users/benjamn/node_modules/recast/lib/lines.js:107:46)
    at new Parser (/Users/benjamn/node_modules/recast/lib/parser.js:11:36)
    at exports.run (/Users/benjamn/node_modules/recast/main.js:13:22)
    at fs.readFile (fs.js:176:14)
    at Object.oncomplete (fs.js:297:15)

For example, we currently consider the indented comment below to be a trailing comment for a();, because it is indented more deeply than the b(); node:

a(); // trailing comment for a()

  // oddly indented comment that should lead b() rather than trailing a()
b();

Need a command line interface

Moving https://github.com/benjamn/ast-types into its own project has worked out very nicely, particularly since it's now easier to distinguish Recast-specific issues from ast-types-specific issues. Let's do the same thing with recast("visitor").Visitor.

Recast does not care what tool you use to manipulate the AST returned by require("recast").parse, so it might be wise to avoid the appearance of preferring one visitor/transformer tool over others.

Note that lib/visitor.js is the only consumer of lib/Class.js, which probably should also be spun off into its own project.

Currently you can define visitor methods like visitForStatement that will be matched whenever a node with .type === "ForStatement" is encountered, but there's no way (short of overriding and reimplementing genericVisit) to define a method like visitStatement.

Ideally, you could have a method like visitStatement together with methods like visitWhileStatement, and the most specific one would be preferred. So WhileStatements would be handled by visitWhileStatement but all other statements would be handled by visitStatement.

One cool possibility that would arise from this change would be that genericVisit could be renamed/aliased to visitNode.

These objects will remain accessible via require("recast").types.{namedTypes,builder,builders}.