Prefer local prettier over bundled version #104

charypar · 2017-03-27T21:09:16Z

Following on the conversation in #59, this is the first cut of the implementation.

Calling prettier through the CLI works fine, but does introduce a short but noticeable lag (~300 ms on a MacBook Pro, so probably even more significant elsewhere). This is mentioned as a potential issue in prettier/prettier#918.

@robwise Is this a significant enough performance penalty to go back to dynamic require until jsonrpc or something similar is introduced in prettier?

I still need to implement the configuration option requested in #43.

to do

Prefer local prettier over bundled version
Error handling
~~Add configuration to only use local prettier and do nothing if there isn't one~~

codecov-io · 2017-03-27T21:11:13Z

Codecov Report

Merging #104 into master will increase coverage by 1.27%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #104      +/-   ##
==========================================
+ Coverage   78.84%   80.12%   +1.27%     
==========================================
  Files           6        6              
  Lines         156      166      +10     
==========================================
+ Hits          123      133      +10     
  Misses         33       33

Impacted Files	Coverage Δ
src/executePrettier.js	`100% <100%> (ø)`	⬆️
src/helpers.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a38ea92...26af8d4. Read the comment docs.

robwise

This is great stuff! Don't forget to add yourself to the contributors list (check nps)!

I left some minor style comments, but the code LGTM. Should we leave the option (whether it should fallback or not) for a new PR and merge this?

Calling prettier through the CLI works fine, but does introduce a short but noticeable lag (~300 ms on a MacBook Pro, so probably even more significant elsewhere). This is mentioned as a potential issue in prettier/prettier#918.

Are you referring to the first time you call Prettier in Atom (this lag already happens even before this PR), or does this happen every time now?

@robwise Is this a significant enough performance penalty to go back to dynamic require until jsonrpc or something similar is introduced in prettier?

Hmm, good question here. If it's just the initial/first time, then I think it's fine. If it's every time, I'm not as sure, 300ms sounds like a lot in some contexts, in others it's not even detectable. I'm not really sure on this.

robwise · 2017-03-28T00:04:21Z

src/helpers.js

+const getPrettierCliOptions = (options: Object) =>
+  Object.keys(options)
+    .reduce((cliOptions, option) => [...cliOptions, `${CLI_OPTIONS[option]} ${options[option]}`], [])
+    .join(' ');


It seems like you're using a reduce to imitate a map? I think you could just use a map directly:

const getPrettierCliOptions = (prettierOptions: {}) => Object.keys(prettierOptions).map(option => `${CLI_OPTIONS[option]} ${prettierOptions[option]`).join(' ');

or you could stay with reduce and skip the array/join:

const getPrettierCliOptions = (prettierOptions: {}) => Object.keys(prettierOptions).reduce((acc, option) => `${acc} ${CLI_OPTIONS[option]} ${prettierOptions[option]`);

Also, I think we maybe should just take the editor as an argument and call prettierOptions ourselves instead of making the caller do it, that way the caller is less coupled:

const getPrettierCliOptions = (editor: TextEditor) => Object.keys(getPrettierOptions(editor)).reduce((acc, option) => `${acc} ${CLI_OPTIONS[option]} ${prettierOptions[option]`);

Ohh 🤦‍♂️ this is what happens when I write code in the evening. This used to concatenate strings like your second suggestion, that's why reduce, then I changed my mind to do an array and a join and didn't notice it's now just a map.

Happy to change it to take editor instead of options.

robwise · 2017-03-28T00:06:51Z

src/helpers.test.js

+
+  test('translates complex options', () => {
+    const expected = '--single-quote true --print-width 100 --trailing-comma es5';
+    const actual = getPrettierCliOptions({ singleQuote: true, printWidth: 100, trailingComma: 'es5' });


this is nit-picky, but can we reverse the order of expected and actual to match above?

robwise · 2017-03-28T00:07:47Z

src/helpers.test.js

+    const actual = getPrettierCliOptions({});
+
+    expect(actual).toEqual(expected);
+  });


I don't think this case should ever occur, should it? We're always going to retrieve all options, right?

Happy to remove that, that's a relic of my test-driving the implementation :)

robwise · 2017-03-28T08:01:02Z

I just read about this in the prettier-atom RFC for linter integration. I'm not totally sure I understand all of this properly.

charypar · 2017-03-28T08:22:21Z

I thought about this some more over night and I think we could solve the performance problem by having the prettier process running in the background and streaming input/output. That way, we'd only pay the penalty once when starting the process and from then on it would be quick. If I imagine it correctly that is.

I also realised I haven't done any error handling for the CLI call. Added it in the to do list at the top.

charypar · 2017-03-28T08:24:09Z

Hmm, good question here. If it's just the initial/first time, then I think it's fine. If it's every time, I'm not as sure, 300ms sounds like a lot in some contexts, in others it's not even detectable. I'm not really sure on this.

It is every time now. I'm not very happy with it as is.

Should we leave the option (whether it should fallback or not) for a new PR and merge this?

Up to you, happy to do either.

robwise · 2017-03-28T08:30:50Z

I also realised I haven't done any error handling for the CLI call. Added it in the to do list at the top.

Yeah, now that I think of it, it may be very difficult to parse the stack trace/error that comes out of the CLI. When you combine that with the performance hit and the complication of needing to run it in the background (which is sort of begging for memory bloat if not leaks IMO), maybe it's better to go with programatically invoking it instead of using the CLI?

robwise · 2017-03-28T08:32:04Z

Up to you, happy to do either.

In that case, can you do a separate PR when you get to that step? That way the PRs are nice and tight! BTW, anytime you want to pair or just if you feel like it's too much work and want to hand it off, let me know, I really appreciate all of this help.

charypar · 2017-03-28T08:37:16Z

Yeah, now that I think of it, it may be very difficult to parse the stack trace/error that comes out of the CLI. When you combine that with the performance hit and the complication of needing to run it in the background (which is sort of begging for memory bloat if not leaks IMO), maybe it's better to go with programatically invoking it instead of using the CLI?

I think running it as a child process, while it requires some work, will work best - no dynamic requires, no startup penalty and we can make sure to only ever have one instance running (closing the previous one if there is one).

You're right the error output could be difficult to parse, but as it stands, I don't think the error reporting is parsing the message at all, does it? It's just printed into an alert.

I'll give it a couple more hours tonight and if it gets crazy, lets go with a dynamic require. How does that sound?

In that case, can you do a separate PR when you get to that step?

Sure, will open as a separate PR.

robwise · 2017-03-28T08:47:11Z

Sounds good to me!

charypar · 2017-03-28T13:59:53Z

I looked into whether the streaming will work and I don't think it will without changes to prettier itself - it currently waits for stdin to close before attempting to format and returning any output.

So that settles it - it's got to be a dynamic require based on how eslint / prettier-eslint does things. Oh well :)

robwise · 2017-04-10T14:20:23Z

@charypar Should I pick this one up?

charypar · 2017-04-10T17:24:22Z

@robwise Oh, thanks for the reminder. Past couple weeks were busy, but I've got time tonight, I'll make the changes and push them in a couple hours.

To make prettier-atom work nicer in projects with prettier installed, it will now prefer the local version of the prettier package over the bundled dependency, to give output consistent with package scripts and CLI prettier. If no local prettier can be found, we fallback to the bundled package.

charypar · 2017-04-10T19:16:24Z

@robwise Ok, I think this is ready to merge, if you're happy with it.

Special error handling is no longer needed, errors can be handled the same way for both bundled and local prettier.

Speaking of errors, would you be interested in a PR that would show parse errors from prettier inline using atom-linter, like eslint does?

robwise

This is awesome! I just had a couple of code style comments/requests.

robwise · 2017-04-11T14:41:25Z

src/executePrettier.js

+      return prettier.format(text, prettierOptions);
+    }
+
+    return getLocalPrettier(localPrettier).format(text, prettierOptions);


Code style comment: I think there's more than one responsibility going on in this function. This function is supposed to be responsible for executing prettier, but now it also has the responsibility for figuring out which prettier to use and where to get it.

I think it would be better, therefore, if we made a helper method simply called getPrettier that would handle all of this internally. This would also make mocking much easier because we can just mock getPrettier.

Sure, that makes more sense, we can then just past the prettier instance to use to the executePrettier function.

We could even pass prettier down the execute stack from format / formatOnSave, like the editor.

I don't know if I'd do the format thing, then you're putting in the logic in two different places that otherwise had no need to know anything about how executePrettier works?

You could move it all the way up to main to do it in one place and pass it through both format functions, I suppose?

Although I guess we might as well hide the problem in the helpers by wrapping it in one function :)

robwise · 2017-04-11T14:46:28Z

src/helpers.js

+
+  const indexPath = path.join('node_modules', 'prettier', 'index.js');
+  const dirPath = getDirFromFilePath(filePath);
+  if (!dirPath) return null;


Is this guard clause possible to hit? If not, you can go FP and avoid state:

const getLocalPrettierPath = (filePath: ?FilePath): ?FilePath => filePath ? findCached(getDirFromFilePath(filePath), path.join('node_modules', 'prettier', 'index.js')) : null;

Also, see my comment in executePrettier.js. There's really no reason to not just give the prettier instance itself instead of making executePrettier do it.

Yep, makes sense, ternary it is. The reason to split the path/lookup logic from the actual requiring is it makes it easier to test in isolation.

robwise · 2017-04-11T14:48:54Z

src/helpers.test.js

+    const expectedLib = path.join('node_modules', 'prettier', 'index.js');
+
+    expect(atomLinter.findCached).toHaveBeenCalledWith(expectedDir, expectedLib);
+  });


I think this test seems to pretty much be checking the same thing as the one above? Should we just keep one?

One is checking we get the right thing back given findCached returns it, the other checks we call findCached with the right arguments.

I had them in one test at first, but this way they can fail independently. Happy to merge them together if you'd prefer.

One is checking we get the right thing back given findCached returns it, the other checks we call findCached with the right arguments.

Yeah although I'd argue the first one is what we need the function to do, and the second one is just an implementation detail, so I think we can drop that one.

robwise · 2017-04-11T14:50:11Z

Speaking of errors, would you be interested in a PR that would show parse errors from prettier inline using atom-linter, like eslint does?

Yeah definitely, that would be awesome.

charypar · 2017-04-11T15:09:49Z

I'll make those changes tonight, should be quick.

charypar · 2017-04-11T21:44:55Z

@robwise This should address all the review comments. Please let me know if you want to change anything else.

Move the logic of loading prettier (either local or global) to the helpers.js to simplify executePrettier.

robwise · 2017-04-12T14:59:51Z

Awesome! Thanks!

robwise approved these changes Mar 28, 2017

View reviewed changes

charypar force-pushed the prefer-local-prettier branch from 7b8c0b1 to 98e2ea9 Compare April 10, 2017 19:09

docs(readme): update contributors

54f8550

robwise suggested changes Apr 11, 2017

View reviewed changes

charypar force-pushed the prefer-local-prettier branch from 3b77be4 to 4d21fb2 Compare April 11, 2017 21:37

refactor(useLocalPrettier): Prettier loading logic

26af8d4

Move the logic of loading prettier (either local or global) to the helpers.js to simplify executePrettier.

charypar force-pushed the prefer-local-prettier branch from 4d21fb2 to 26af8d4 Compare April 11, 2017 21:54

robwise approved these changes Apr 12, 2017

View reviewed changes

robwise merged commit cd8185c into prettier:master Apr 12, 2017

robwise mentioned this pull request Apr 13, 2017

Select bin file from repo #59

Closed

charypar mentioned this pull request May 12, 2017

The contribution calculations seem quite off redbadger/karma-tracker#31

Open

darahak mentioned this pull request Jun 3, 2017

Fails on save with EvalError with version 0.32.0 #182

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefer local prettier over bundled version #104

Prefer local prettier over bundled version #104

charypar commented Mar 27, 2017 •

edited

codecov-io commented Mar 27, 2017 •

edited

robwise left a comment •

edited

robwise Mar 28, 2017

charypar Mar 28, 2017

robwise Mar 28, 2017

charypar Mar 28, 2017

robwise Mar 28, 2017

charypar Mar 28, 2017

robwise commented Mar 28, 2017

charypar commented Mar 28, 2017

charypar commented Mar 28, 2017

robwise commented Mar 28, 2017

robwise commented Mar 28, 2017

charypar commented Mar 28, 2017

robwise commented Mar 28, 2017

charypar commented Mar 28, 2017

robwise commented Apr 10, 2017

charypar commented Apr 10, 2017

charypar commented Apr 10, 2017

robwise left a comment

robwise Apr 11, 2017

charypar Apr 11, 2017

robwise Apr 11, 2017

charypar Apr 11, 2017

robwise Apr 11, 2017

charypar Apr 11, 2017 •

edited

robwise Apr 11, 2017

charypar Apr 11, 2017

robwise Apr 11, 2017

robwise commented Apr 11, 2017

charypar commented Apr 11, 2017

charypar commented Apr 11, 2017

robwise commented Apr 12, 2017

Prefer local prettier over bundled version #104

Prefer local prettier over bundled version #104

Conversation

charypar commented Mar 27, 2017 • edited

to do

codecov-io commented Mar 27, 2017 • edited

Codecov Report

robwise left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robwise commented Mar 28, 2017

charypar commented Mar 28, 2017

charypar commented Mar 28, 2017

robwise commented Mar 28, 2017

robwise commented Mar 28, 2017

charypar commented Mar 28, 2017

robwise commented Mar 28, 2017

charypar commented Mar 28, 2017

robwise commented Apr 10, 2017

charypar commented Apr 10, 2017

charypar commented Apr 10, 2017

robwise left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charypar Apr 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robwise commented Apr 11, 2017

charypar commented Apr 11, 2017

charypar commented Apr 11, 2017

robwise commented Apr 12, 2017

charypar commented Mar 27, 2017 •

edited

codecov-io commented Mar 27, 2017 •

edited

robwise left a comment •

edited

charypar Apr 11, 2017 •

edited