millermedeiros / esformatter

ECMAScript code beautifier/formatter
MIT License
970 stars 91 forks source link

Adds sort & rearrange code #215

Open eduardolundgren opened 10 years ago

eduardolundgren commented 10 years ago

Sorting and rearranging your code helps to keep your code organized. Sorted code makes is easier to maintain and to navigate in. Whether you group related members based on functionality or sort members based on entity type, visibility and name. Would be good if esformatter have such functionality.

pgilad commented 10 years ago

Is this related to sorting object properties or pre-declared variables/functions? If it's related to actual code I don't see how you can be sure you aren't hurting logic

jzaefferer commented 10 years ago

Similar to variable renaming (#214) this is out of scope for esformatter, which is already complex enough. The same underlying tools can be used to implement that though, like rocambole.

eduardolundgren commented 9 years ago

That would be an amazing feature to have in a great tool like esformatter, though indeed make sense to be out of esformatter scope.

Thanks @jzaefferer.

millermedeiros commented 9 years ago

reopening since we decided to not close plugins-wishlist bugs until they are implemented

eduardolundgren commented 9 years ago

:+1:

davidpfahler commented 8 years ago

Is this deemed "impossible" with the current setup of esformatter or can such a plugin be done?

davidpfahler commented 8 years ago

@millermedeiros In the blog post you link to from the README.md you say:

Nowadays I believe that a reliable code formatter should not rewrite the original source code, it should only modify the white spaces and line breaks while keeping all the original tokens (only non-destructive transformations).

I would like to know if this is still your position and - if so - if my understanding is correct that this will prevent esformatter from ever supporting features like variables renaming, variable and import sorting and object property sorting.

Fyi, my background for this question is the following: I've been looking into finding/creating a formatter for JavaScript which corresponds to gofmt for golang. For me, this means it should support all of the above named features (and more). In an ideal world, this formatter would also support a plugins system where each plugin corresponds to an existing ESLint rule (which it autofixes when violated). This would allow anyone to create their own version of their gofmt-equivilant formatter. For example, their could be a standard preset that tries to unify the community, but also any collection of plugins or differences from any preset, much like the situation is today with standard and ESLint. It appears to me that such a formatter cannot possible be implemented with the current esformatter's architecture and principles.

millermedeiros commented 8 years ago

@davidpfahler it is definitely possible, we have the stringBefore and stringAfter API for plugins, exactly to allow destructive transforms (also the pipe option)

a few plugins that does destructive manipulations:

for a plugin like this (sorting imports, object properties, etc...), I would recommend using something like recast under the hood (see ASTExplorer jscodeshift transform - sorting object props is really trivial to implement with recast... - and execute it as a stringBefore or pipe.before

the thing is: I won't introduce any kind of destructive manipulation to the core project (esformatter should only focus on white space, line breaks and indentation), but I will encourage people to write those features as plugins (and I'll even write some).

davidpfahler commented 8 years ago

@millermedeiros Thank you for that instructive answer! I hope you don't mind me following up on this right here: The API for plugins docs (https://github.com/millermedeiros/esformatter/blob/master/doc/plugins.md) recommend on multiple occasions to use stringBefore method or the pipe option for destructive (or rather mutative; is that the right word?) AST operations. That seems rather wasteful as parsing needs to be done multiple times. I don't get how it is safer if my standalone CLI parses code to AST, transforms th AST and wrotes back to string again, which is the parsed to AST by esformatter. Why can I not supply my transformed AST directly to esformatter, without rendering back to a string?

millermedeiros commented 8 years ago

@davidpfahler it's indeed wasteful, but parsing of small files is fast. The main reason for that design decision was because it's too hard to generate a compatible AST + token list (and it would still need multiple passes to make sure the changes didn't conflict with other plugins).

also, because of the way rocambole.moonwalk is implemented, nodes added during the loop are going to be ignored, elements removed during the loop are still processed. (this is needed for a HUGE perf boost)

we could potentially add something similar to stringBefore and stringAfter to process the AST instead of the string; but right now moonwalk relies on the depth property and there is a lot weird/complex logic on how we instrumentNodes and get the start and end tokens... (logic inside rocambole.parse) - I decided that it would be too hard for someone to generate the proper structure and decided that it was premature optimization...

see also https://github.com/millermedeiros/esformatter/issues/86

davidpfahler commented 8 years ago

@millermedeiros Thanks for the explanation. I will try how much of an issue perf is and review this later. However, there is still one thing I don't quite get and I'd really appreciate it if you could enlighten me on this point: As far as I know, esformatter uses babylon to parse the source string to an AST. Now if I apply a transform to that AST before anything is done to it by esformatter or rocambole (or whatever), how could this break anything? I can't imagine esformatter could even know it received a modified AST.

millermedeiros commented 8 years ago

@davidpfahler if you mutate the AST, in a way that is compatible with the rocambole AST, and call esformatter.transform it should work fine.

The thing here is that rocambole adds a lot of stuff that is not available on the Babylon/Espree/Esprima ASTs (eg. startToken, endToken, depth, next...); as soon as rocambole.moonwalk is called adding/removing nodes causes unexpected behavior (because we cache the original nodes before starting the loop to boost performance) - the way indentation works is also very easy to mess up (we add a level property to the Indent nodes and remove any whitespace at the beginning of the lines that doesn't have this prop) - so any transform that changes the program structure, should be done as a separate parse step.

PS: there are some cool recast/jscodeshift transforms that could be reused https://github.com/cpojer/js-codemod

davidpfahler commented 8 years ago

@millermedeiros Thanks. But there is currently no hook that allows me to modify the AST before esformatter.transform is called? Or is that transformBefore?

millermedeiros commented 8 years ago

@davidpfahler currently there is no hook to allow that; introducing the hook is simple, the hard part is returning the proper AST from the plugins.

I would be open to the new hook if you have good use cases, but I still think that stringBefore + recast is going to be simpler for any mutative/destructive transform. - In some cases, adding a single ; changes the whole program structure.