Support better formatting for curried arrow functions

[sosukesuzuki]

Description

Fixes #9985

Checklist

• I’ve added tests to confirm my change works.
• (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
• I’ve read the contributing guidelines.

✨Try the playground for this PR✨

[fisker]

This seems not pretty.

[fisker]

Maybe no break before first level parameters more reasonable.

[sosukesuzuki]

what do you mean? Should be like below?:

fooooooooooooooooooooooooooooooooooooooooooooooooooo((action) =>
  (next) => dispatch(action)
);

[fisker]

Yes, I mean this, but I'm not sure. Better idea?

[thorn0]

I'd prefer not breaking the chain if it fits on one line:

fooooooooooooooooooooooooooooooooooooooooooooooooooo(
  (action) => (next) => dispatch(action)
);

And if the chain is long enough, then:

fooooooooooooooooooooooooooooooooooooooooooooooooooo(
  (action) =>
    (next) =>
    (next) =>
    (next) =>
    (next) =>
    (next) =>
    (next) => 
    dispatch(action)
);

This is similar to how Prettier formats binary expressions in call arguments.

[fisker]

In that case, assign print like this?

const foo = (action) =>
  (next) =>
  (next) =>
  (next) =>
  (next) =>
  (next) =>
  (next) => 
  dispatch(action)

[thorn0]

Assignments are complicated. To make the chain look uniform and to avoid dealing with edge cases (#2482), it's better to break after =.

const foo =
  (action) =>
  (next) =>
  (next) =>
  (next) =>
  (next) =>
  (next) =>
  (next) => 
  dispatch(action)

[sosukesuzuki]

Thank you. I agree with thorn0's suggestions.

[sosukesuzuki]

Can anyone review?

[fisker]

I know this is silly, but can we add import() too?

[fisker]

Prettier pr-9992
Playground link

--parser babel
--prose-wrap always

Input:

import( (argument1) =>
    (argument2) =>
    (argument3) =>
    (argument4) =>
    (argument5) =>
    (argument6) =>
    (argument7) =>
    (argument8) =>
    (argument9) =>
    (argument10) =>
    (argument11) =>
    (argument12) => {
      const foo = "foo";
      return foo + "bar";
    });

Output:

import(
  (argument1) =>
    (argument2) =>
    (argument3) =>
    (argument4) =>
    (argument5) =>
    (argument6) =>
    (argument7) =>
    (argument8) =>
    (argument9) =>
    (argument10) =>
    (argument11) =>
    (argument12) => {
      const foo = "foo";
      return foo + "bar";
    }
);

Prettier pr-9992
Playground link

--parser meriyah
--prose-wrap always

Input:

import( (argument1) =>
    (argument2) =>
    (argument3) =>
    (argument4) =>
    (argument5) =>
    (argument6) =>
    (argument7) =>
    (argument8) =>
    (argument9) =>
    (argument10) =>
    (argument11) =>
    (argument12) => {
      const foo = "foo";
      return foo + "bar";
    });

Output:

import(
  (argument1) =>
  (argument2) =>
  (argument3) =>
  (argument4) =>
  (argument5) =>
  (argument6) =>
  (argument7) =>
  (argument8) =>
  (argument9) =>
  (argument10) =>
  (argument11) =>
  (argument12) => {
    const foo = "foo";
    return foo + "bar";
  }
);

[sosukesuzuki]

We cannot add ImportExpression to isCallLikeExpression because import(...) isn't like call expression. But I changed to format import and curried functions in the same way as call expressions ( 2de96ba ).

[fisker]

Sorry, didn't see this, I think we should, we already treat it like CallExpression everywhere.

[sosukesuzuki]

What do you think about #9992 (comment) ?

[fisker]

This why I added

prettier/src/language-js/utils.js

Lines 1210 to 1235 in 2f3fc24

	const callArgumentsCache = new WeakMap();
	function getCallArguments(node) {
	if (callArgumentsCache.has(node)) {
	return callArgumentsCache.get(node);
	}
	const args =
	node.type === "ImportExpression"
	? // No parser except `babel` supports `import("./foo.json", { assert: { type: "json" } })` yet,
	// And `babel` parser it as `CallExpression`
	// We need add the second argument here
	[node.source]
	: node.arguments;
	callArgumentsCache.set(node, args);
	return args;
	}
	function iterateCallArgumentsPath(path, iteratee) {
	const node = path.getValue();
	// See comment in `getCallArguments`
	if (node.type === "ImportExpression") {
	path.call((sourcePath) => iteratee(sourcePath, 0), "source");
	} else {
	path.each(iteratee, "arguments");
	}
	}

, should not use node.arguments.

[sosukesuzuki]

Okay, I'll fix.

[sosukesuzuki]

done 6fb35ae

[thorn0]

I need some more time to look into this. Sorry for the delay.

[thorn0]

I'm afraid this change might be too drastic. Before merging we need to test it on a couple of codebases that heavily use curried arrow functions. Anybody know of such projects we can use for testing?

[thorn0]

Looks very good overall. I have comments on two things.

(1) What's the story behind this difference?

Prettier pr-9992
Playground link

--parser babel

Input:

const curryTest =
    (argument1) =>
    (argument2) => ({
        foo: argument1,
        bar: argument2,
    });

const curryTest =
    (argument1) =>
    (argument2) =>
    (argument3) => ({
        foo: argument1,
        bar: argument2,
    });

Output:

const curryTest =
  (argument1) =>
  (argument2) => ({
    foo: argument1,
    bar: argument2,
  });

const curryTest =
  (argument1) =>
  (argument2) =>
  (argument3) =>
  ({
    foo: argument1,
    bar: argument2,
  });

(2) After playing with different examples, I decided that after all it's better to add another level of indentation to the body when the body is an expression.

const curryTest =
  (argument1) =>
  (argument2) =>
  (argument3) =>
    argument1 + argument2 + argument3;

const foo =
  (action) =>
  (next) =>
  (next) =>
  (next) =>
  (next) =>
  (next) =>
  (next) =>
    ({
      foo: argument1,
      bar: argument2,
    });

Looks a bit better to me than the current output:

const curryTest =
  (argument1) =>
  (argument2) =>
  (argument3) =>
  argument1 + argument2 + argument3;

const foo =
  (action) =>
  (next) =>
  (next) =>
  (next) =>
  (next) =>
  (next) =>
  (next) =>
  ({
    foo: argument1,
    bar: argument2,
  });

.

[thorn0]

@sosukesuzuki I can't request your review because it's your PR, but please take a look. I tried to minimize the diff in prettier/prettier-regression-testing#40 and only fix what was reported in #9985.

A remaining issue is the fact that I don't know how to keep this formatting: (fixed!)

request.get("https://preview-9992--prettier.netlify.app", (head) => (body) => {
  console.log(head, body);
});

I'm not sure we have to keep it, but it looks intended.

[fisker]

LGTM

[sosukesuzuki]

I can't approve this PR on GitHub because I opened this.

But LGTM! Thank you!

[thorn0]

Oops. Sure, it's a corner case, but assignment chains and arrow chains don't play well together:

Prettier pr-9992
Playground link

--parser babel
--print-width 75

Input:

a=b=d=(
  argumentOne,
  argumentTwo,
  argumentThree
) => restOfTheArguments123 => {
  return "baz";
}

Output:

a =
  b =
  d =

      (argumentOne, argumentTwo, argumentThree) =>
      (restOfTheArguments123) => {
        return "baz";
      };

[onhate]

how do I rollback to the previous behavior?

[tony]

Am I correct in this not being toggleable / configurable? I'm surprised by how much this indentation to swaths of our codebase 😆

Personally I'm fine with the formatting (whatever's consistent) but I think something like this needs an "opt out" so people could upgrade prettier and delay the change.

In the README, it'd be good to get more details on if this is tweakable somehow.

[sosukesuzuki]

You(users) cannot rollback to prev behavior. But if you have actual code examples made harder to read by this feature, you can report it as a bug. And it'll help us to improve formatting heuristics.

[codeaid]

I upgraded to 2.3 earlier today and after running prettier --check it showed me around 20 files that are apparently incorrect despite being just fine before that. I suspect it has something to do with this PR. From what I gather there is no way to see what is actually wrong with them without running --write (convenient, not) but I eventually managed to find the problem:

export const createCombinedReducer = (): Reducer<State> =>
    // Wrap root reducer to handle store reset actions
    (state, action) => {
        if (action.type === SystemAction.Reset) {
            return rootReducer(undefined, action);
        }

        return rootReducer(state, action);
    };

The reformatted version (100 chars wide) looks like this, which in my view is much uglier than before:

export const createCombinedReducer =
    (): Reducer<State> =>
    // Wrap root reducer to handle store reset actions
    (state, action) => {
        if (action.type === SystemAction.Reset) {
            return rootReducer(undefined, action);
        }

        return rootReducer(state, action);
    };

If I remove the comment from line 3 it becomes something totally different again:

export const createCombinedReducer = (): Reducer<State> => (state, action) => {
    if (action.type === SystemAction.Reset) {
        return rootReducer(undefined, action);
    }

    return rootReducer(state, action);
};

Is there a flag to disable moving (): Reducer<State> => to a new line or is the only way to keep the previous functionality is to lock the version to 2.2?

P.S.
I'm rather unhappy with this change because it seems to be a rather big one. It's already 20 or more files In my relatively small codebase and I can only imagine what a headache it would be if I had more files. Opinionated is good but prettier being so ingrained in the current development stack shouldn't have minor versions making such drastic changes. The 2.3 changelog is absolutely huge, which makes me wonder - why wasn't it 3.0?