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

Why add new tokens instead of purely decorating existing structures? #7

Closed
bitwiseman opened this issue Jun 4, 2013 · 3 comments
Closed

Comments

@bitwiseman
Copy link

Just wondering if you could comment on why you chose to add new tokens to handle comments and whitespace, instead of purely decorating existing tokens or nodes with additional information?

By adding new tokens you make the output of rocambole incompatible with tools that consume esprima output. You also make processing of the output more complex.

For example, most formatting rules are based on what node a token is inside and sometimes what token came before them. On js-beautify, we found that comments and whitespace are secondary infomation to rules - keeping them as a sideband of information makes rules simpler while still letting the cases where they matter function.

Another example is the UglifyJs token structure. It adds the comment tokens as decoration for regular tokens.

@millermedeiros
Copy link
Owner

my idea was mainly because in some cases line breaks should be kept/removed based on the comments.. and grabbing the Comments/LineBreaks inside a node is simpler if it is already inside the tokens list.

if you search inside esformatter code, you will see that I use isComment in many places same thing for LineBreak tokens.

it's also way easier to rebuild the JS string if the tokens already have line breaks and comments. Also easier to identify if previous/next/current token is a LineBreak or Comment.

@millermedeiros
Copy link
Owner

But you are right.. I could have just added the new properties and kept the tokens array with the original data and have the LineBreak and Comments just on the linked list. Maybe on a future version.

@bitwiseman
Copy link
Author

Cool thanks, you might want to put this on the project wiki. It a valid design choice, probably not valid for the readme abut worth having documented as something other than an issue.

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

2 participants