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

Initial attempt at chaining call expressions #4

Merged
merged 3 commits into from Jan 8, 2017
Merged

Conversation

jlongster
Copy link
Member

This seems to work pretty well, however this forces simple expressions that have a CallExpression on the left-hand to break on a newline:

foo()
  .callMethod()

The question is how can we differentiate between that and places we want to chain?

@jlongster jlongster changed the title Initial attempt at chaining call expression chaining Initial attempt at chaining call expressions Jan 5, 2017
@vramana
Copy link

vramana commented Jan 6, 2017

Does this convert

const a = foo.callMethod();

to

const a = (
  foo
    .callMethod()
);

Edit: Sorry forgot to look at tests.

@jlongster
Copy link
Member Author

Does this convert...

I don't know what the previous one did, but I just pushed a new commit that is much better, and it does not.

@jlongster
Copy link
Member Author

I just updated my heuristic for this. I'm pretty happy with it, but I've been pulling my hair out for a while trying to figure it out. It's pretty hard because of the amount of variations and subjectivity involved.

You can see a big file with all sorts of variations and the result of my printer: https://gist.github.com/jlongster/e17d35d03e565c2858e609af872db66b

My heuristic boils down to this: if there are successive CallExpression's on MemberExpression's, print them as a chain if there 2 or more functions passed in as the first arg in any of the calls. The point is that you really want it printed as a chain if you are doing this style of code:

arr
  .map(x => x + 1)
  .filter(x => x > 10)
  .some(x => x % 2)

And that's a simple example, if you have bigger functions inline it must be printed like this. However, if you didn't inline the functions, or the functions take some other kind of argument, you want it printed like this:

arr.map(foo).filter(bar).some(baz);

The reason we require 2 or more is because things like this is a pretty common pattern:

Object.keys(foo).forEach(x => {
  return x
});

That still prints the same way with my printer. Once you pass 2 functions in, though, it would bump each lookup onto a newline.

Note that when it prints a chain, it always prints a newline before the .. That's why the first map in the first example above is on a newline, which some people might not like, but it's the most consistent and not really a big deal. This does hit a weird case though if the initial expression is a function call like this:

  fetch(url, {
    method: "post",
    foo: "bar",
    credentials: "include",
    headers: {"Accept": "contsdlfkjsdfkldjs", "Accept2": "contsdlfkjsdfkldjs"}
  })
    .then(resp => resp.json())
    .then(obj => callback(obj))
    .then(obj => callback(obj))
    .then(obj => callback(obj));

I think that's OK though, and is perfectly consistent will everything else. Some people might think it looks weird, but that's subjective and we need to settle somewhere.

The last thing my commit does is make sure so split a long list of lookups/calls together if it's too long. Meaning this:

foo(foo).forEach().filter().foo().bar().forEach().filter().foo().bar().forEach().filter().foo().bar();

will be split up as this:

foo(foo)
  .forEach()
  .filter()
  .foo()
  .bar()
  .forEach()
  .filter()
  .foo()
  .bar()
  .forEach()
  .filter()
  .foo()
  .bar();

I'm done thinking about this. :) Please run it on existing code and let me know how it looks. We can tweak it later, but this is at least a good starting point.

@jlongster jlongster merged commit 8a5f3d1 into master Jan 8, 2017
@jlongster jlongster mentioned this pull request Jan 8, 2017
13 tasks
@jlongster jlongster deleted the call-chain branch January 9, 2017 14:46
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 21, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants