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

Control of empty lines #147

Open
ptbrowne opened this issue Feb 1, 2015 · 13 comments
Open

Control of empty lines #147

ptbrowne opened this issue Feb 1, 2015 · 13 comments

Comments

@ptbrowne
Copy link

ptbrowne commented Feb 1, 2015

Hi,

I have just made a recast script and it is working well but I have a final request that I did not manage to solve. I wonder if it possible.

My script reorders requirejs define's calls according to a giving priority.

define([
  'views/1',

  'models/2',
  'views/3',
], function (
  v1,
  m2,
  v3
) {
  // BODY
});

with priority [ 'views', 'models' ] is transformed into

define([
  'views/1',

  'views/3',
  'models/2'
], function (
  v1,
  v3,
  m2
) {
  // BODY
})

The only problem here is that recast is smart enough to reuse the whitespace so it puts an empty line between views/1 and views/3 but I want to put this empty line between views/3 and models/2. I was thinking of tricking recast by modifying the node.original object but skimming through the source code of lines.js, I saw that lines are supposed to be immutable.

Is there a proper way to do that ? I have tried with reuseWhitespace set to false but to no avail..

Thanks for recast, it is awesome 👍

My complete script is here : https://gist.github.com/ptbrowne/2b5e9682698cdd5316f1

@benjamn
Copy link
Owner

benjamn commented Feb 4, 2015

Hmm, I wonder if we could support attaching a custom property like .leadingSpace to AST nodes, and then the printer would pay attention to that (when present) instead of trying to reuse the original spaces.

Note that, currently, the space preservation logic only works for lists of statements: https://github.com/benjamn/recast/blob/master/lib/printer.js#L1078-L1092

It might be worth supporting space preservation and .leadingSpace for other kinds of multiline sequences, like ArrayExpression elements, ObjectExpression properties, and Function parameters.

@treshugart
Copy link

This might be similar to my use case. I'm trying to concatenate files while preserving comments. The problem is as I do something like:

var body1 = ast1.program.body;
var body2 = ast2.program.body;
body1.push.apply(body1, body2);

They get squished together. I'd like to add a bit of whitespace to them and pretty printing does that, but it also removes the comments which are at the top of each concatenated file.

Is there a better way to do this? I've combed the documentation pretty good but I could have missed something.

@benjamn
Copy link
Owner

benjamn commented Jul 30, 2015

@treshugart Ah, that's an interesting use case! I suspect the comments you're talking about are attached as ast2.program.comments, and combining the bodies throws that Program node away.

Maybe try something like this?

var body1 = ast1.program.body;
var body2 = ast2.program.body;

var firstStmt = body2[0];
var prog2Comments = ast2.program.comments;
if (prog2Comments && firstStmt) {
  firstStmt.comments = prog2Comments.concat(firstStmt.comments || []);
}

body1.push.apply(body1, body2);

@treshugart
Copy link

Hmm, the comments are preserved if I don't pretty print, but what I really want to do is put some whitespace between the two. Taking my earlier example:

var body1 = ast1.program.body;
var body2 = ast2.program.body;

// I know whitespace isn't really a valid token in ESTree, but Recast maintains
// the original formatting, so I was hoping something like this could be achieved.
body1.push('\n', '\n');

body1.push.apply(body1, body2);

@benjamn
Copy link
Owner

benjamn commented Jul 30, 2015

Another option is just to recast.print the two programs separately, and manually join the strings together. For what it's worth, adding strings directly to the body is fine as far as Recast is concerned, though it might cause you problems if you try to run another transform after doing that.

@treshugart
Copy link

My main concern is keeping the original references to AST nodes so that I can generate source maps from the concatenated source from recast.print(). Not sure how I'd combine the two source maps together if I separately printed them.

@treshugart
Copy link

Anyways, I don't want to detract from the main issue too much. Thanks again for your help.

@pieterv
Copy link

pieterv commented Aug 5, 2015

This has been a blocker for us trying to create an auto formatter for require() blocks. I was able to add newlines between statements by inserting a unique identifier then replacing the string output with a \n and i was able to remove newlines on existing statements by recreating the statements but then the var {a1, a2} = require('a'); would turn into var {\n a1, \n a2 \n} = require('a') which is kinda ugly. Being able to easily set the whitespace via .leadingSpace would be hugely helpful :)

@benjamn
Copy link
Owner

benjamn commented Nov 2, 2015

This still seems worthwhile (cf. https://twitter.com/phuunet/status/661246993822580736), and I hope to find some time to work on it soon.

If we support both .leadingSpace and .trailingSpace, then I think the actual space between two adjacent nodes should be the maximum of the spaces, rather than a concatenation, so that if one node has .trailingSpace === "\n" and the following node also has .leadingSpace === "\n", then there actually ends up being only one newline between them.

Another question: is there every any reason to add whitespace other than newlines? If not, then we could go with numeric properties like .leadingLineCount and .trailingLineCount.

@qfox
Copy link

qfox commented Apr 23, 2016

Unfortunately, these properties looks like a hack for the concrete cases. Are you really sure you want to keep trying it with just AST?

Since there are already some implementations of CST:

@jamestalmage
Copy link
Contributor

This issue predates both those CST implementations. A good CST implementation is probably the optimal way forward, but they are a little immature to introduce into a recast, which is heavily relied upon. One is a PR, and the other is still version 0.1. That said, cst looks really promising.

It would be really interesting to modify ast-types to work with cst. It should really only require re-implementing the modifier methods (.replace, etc) so they defer to the cst API.

@nickretallack
Copy link

Idk if you care about this, but I found a place where recast is pointlessly removing a newline.

If you have two type declarations separated by a newline and you run it through the babel-plugin-flow-to-typescript conversion it doesn't retain the newline.

I explored that here: zxbodya/flowts#12

@dantman
Copy link

dantman commented Dec 9, 2021

Is there any plans to fix this? I'm having issues with recast inserting unnecessary line breaks and empty lines in object declarations I'm generating. Which Prettier of course won't strip out since recast inserts them the way a user would intentionally add them.

I don't know of any alternative to recast that can be used instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants