Treat define calls like require#3830
Treat define calls like require#3830j-f1 merged 4 commits intoprettier:masterfrom salemhilal:require-define-same-line
Conversation
See issue #3829. This commit keeps define calls as a unit when possible, to prevent an extra indent in the body of an AMD-style define. Rather than adding the "define" check in the same case as the existing "require" check, I added a separate condition to explicitly call out the fact that this checks for both AMD and CommonJS modules. `yarn test -u` yields no changes, and `yarn lint` passes successfully.
j-f1
left a comment
There was a problem hiding this comment.
Thanks for contributing to Prettier! Just one small question.
src/language-js/printer-estree.js
Outdated
| // e.g. `define(["some/lib", (lib) => {` | ||
| (!isNew && | ||
| n.callee.type === "Identifier" && | ||
| n.callee.name === "define") || |
There was a problem hiding this comment.
What do you think about moving this to the previous test?
(!isNew &&
n.callee.type === "Identifier" &&
(n.callee.name === "require" ||
n.callee.name === "define"))There was a problem hiding this comment.
I did that at first; I'm ambivalent there honestly. I ended up with this approach because it allows for a more obvious way to diverge CommonJs calls from AMD calls in the future. Lemme know and I'm down to move it to the previous test.
There was a problem hiding this comment.
Let's move it into the previous test
|
Could you add your input from #3829 as a test? |
This tests the solution for issue #3829.
|
^ tests added |
tests/require-amd/jsfmt.spec.js
Outdated
| @@ -0,0 +1 @@ | |||
| run_spec(__dirname, ["flow", "typescript"]); | |||
|
^ added babylon parser to the test. |
|
Thanks for contributing to Prettier! 🎉 |
|
Woo! Thank you. |
|
Hey, what's the release schedule like for prettier? I'm curious when this patch will be live (without having to checkout master). |
|
We usually do a patch release every month or so, so it's around that time that we should do a new one. |
See issue #3829. This commit keeps define calls as a unit when possible,
to prevent an extra indent in the body of an AMD-style define.
Rather than adding the "define" check in the same case as the existing
"require" check, I added a separate condition to explicitly call out the
fact that this checks for both AMD and CommonJS modules.
yarn test -uyields no changes, andyarn lintpasses successfully.