Fallback to global instance of prettier before falling back to a bundled one #397

kachkaev · 2018-03-22T14:27:52Z

The PR partially addresses #395

The extension now searches for a globally installed prettier if a local one is not found; it falls back to a a bundled instance only if both attempts have failed. This change makes it possible to use Prettier plugins when someone does not want to have any npm modules locally. Please note that Prettier still has issues with detecting plugins in global installs, but I hope it gets fixed soon. IMO it's OK to merge this PR before that happens and simply npm i -g prettier once the upstream fix arrives.

cc @olsonpm

codecov-io · 2018-03-22T15:08:45Z

Codecov Report

Merging #397 into master will increase coverage by 0.47%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #397      +/-   ##
==========================================
+ Coverage    78.6%   79.07%   +0.47%     
==========================================
  Files          27       28       +1     
  Lines         486      497      +11     
  Branches       53       55       +2     
==========================================
+ Hits          382      393      +11     
  Misses         88       88              
  Partials       16       16

Impacted Files	Coverage Δ
src/helpers/getPrettierInstance.js	`100% <100%> (ø)`	⬆️
src/helpers/getPrettierPath.js	`100% <100%> (ø)`
src/displayDebugInfo/index.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 08035ff...d5de657. Read the comment docs.

kachkaev · 2018-03-22T15:09:20Z

I also added printing git diff to the CI script, which should only run when dirty files are found. Hope this helps future contributors!

Had to re-commit four times because checkForDistChanges was failing and I could not figure out why. Tried not to exclude dist and even tweaked this folder manually before simply realising that the problem was with yarn.lock not being up-to-date 😅 My mistake was that I did not notice yarn.lock initially and thought you were using npm. Not a surprise that a new package in package.json made yarn.lock out of sync and was thus making this file dirty in CI.

robwise

Roger that, good idea with the CI script.

I had a question about whether this works with Yarn globally-installed packages?

Also, can you use the conventional-changelog format for your commit message? Without this, the auto-generated changelog won't pick up your changes properly.

robwise · 2018-03-22T21:07:26Z

src/helpers/getPrettierInstance.test.js

@@ -22,6 +23,24 @@ test("returns user's project's local prettier instance if it exists", () => {
  );
 });

+test("returns global prettier if user's project has no local prettier package", () => {
+  atomLinter.findCached.mockImplementation(() => undefined);


this seems to be dead code as you're overriding your mock on line 32?

robwise · 2018-03-22T21:08:14Z

src/helpers/getPrettierInstance.test.js

+test("returns global prettier if user's project has no local prettier package", () => {
+  atomLinter.findCached.mockImplementation(() => undefined);
+  const filePath = path.join(__dirname, 'sourceFile.js');
+  const file = { path: filePath, getPath: () => filePath };


I believe we only need getPath to be defined? Well, doesn't matter I guess

robwise · 2018-03-22T21:11:43Z

src/helpers/getPrettierInstance.js

@@ -4,11 +4,13 @@ const bundledPrettier = require('prettier');
 const { getCurrentFilePath } = require('../editorInterface');
 const { findCachedFromFilePath } = require('./general');
 const path = require('path');
+const { findCached } = require('atom-linter');
+const globalModules = require('global-modules');


Does this work for globally installed packages in Yarn? The repo makes it sound like it only works for npm? We should really support both if possible.

robwise · 2018-03-22T21:17:30Z

src/helpers/getPrettierInstance.test.js

+  const filePath = path.join(__dirname, 'sourceFile.js');
+  const file = { path: filePath, getPath: () => filePath };
+  const editor = createMockTextEditor({ buffer: { file } });
+  const prettierLib = path.join(__dirname, '..', '..', 'tests', 'fixtures', 'prettier.js');


I'm worried that this is a bit confusing because you're using a local prettier instance as a fake prettier instance to use when there is no local prettier instance (it's even confusing to say in English). I think if just renamed it a bit it would be more clear, something like fakeGloballyInstalledPrettier?

olsonpm · 2018-03-22T21:59:15Z

mind adding a line in displayDebugInfo/index.js so we have this available when helping users with bugs?

kachkaev · 2018-03-22T22:05:41Z

@olsonpm which line?

UPD: I think I got it. 5 mins.

olsonpm · 2018-03-22T22:09:34Z

just to be clear, I was just hoping for the version of the user's global prettier if found. If not, I would like that to be explicit as well e.g. "global prettier version: <not found>"

kachkaev · 2018-03-22T22:31:16Z

@olsonpm I'm printing prettier's path in debug info now. Could you please help me with passing editor and thus filePath in there? It'd be great if you could commit a PR on top of mine – I'll be away for some time now.

olsonpm · 2018-03-22T22:34:46Z

If you have prettier's path, then wouldn't you just

const packageJsonPath = path.join(pathToGlobalPrettier, 'package.json')
const globalPrettierVersion = require(packageJsonPath).version

Not sure what you're asking

…rn) before falling back to a bundled one

kachkaev · 2018-03-22T22:50:02Z

Got you. Now finally going to sleep 😅

kachkaev · 2018-03-23T07:40:07Z

@robwise if the PR looks OK to you now after refactoring, it'd really help if you could merge it and release a new version.

Can't wait to start using prettier-plugin-elm inside inside markdown files with colleagues! 😄 After Atom's package supports global prettier, I'll just need to finish one PR for prettier and that'll tick all the missing boxes for us 🎉

robwise · 2018-03-24T01:12:37Z

LGTM!

krugar · 2018-08-22T12:44:24Z

This does not find a globally installed prettier under linux when node is installed via nvm. Might be related to jonschlinkert/global-prefix#19, but I didn't really care to dig, as this does not hamper use of prettier-atom in any way (it falls back gracefully to the bundled prettier).

I just wanted to give you folks a heads up.

robwise · 2018-08-23T02:38:38Z

Thanks for reporting, I will make an issue anyway just in case someone is looking for this info: #448

kachkaev mentioned this pull request Mar 22, 2018

Plugin support? #395

Closed

robwise suggested changes Mar 22, 2018

View reviewed changes

kachkaev added 2 commits March 22, 2018 22:48

feat(prettier) Fallback to globally installed prettier (via npm or ya…

b1fdcd2

…rn) before falling back to a bundled one

ci(travis): show git diff when dist check has failed

d5de657

robwise approved these changes Mar 24, 2018

View reviewed changes

robwise merged commit 8846b75 into prettier:master Mar 24, 2018

kachkaev mentioned this pull request May 11, 2018

Custom Prettier Resolution prettier/prettier-vscode#232

Closed

kachkaev mentioned this pull request Aug 4, 2018

Simplify Settings by Relying on Users Setting Prettier Config #404

Merged

kachkaev mentioned this pull request Oct 4, 2019

fix(global-prettier-resolution): add fallback for NVM users #516

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fallback to global instance of prettier before falling back to a bundled one #397

Fallback to global instance of prettier before falling back to a bundled one #397

kachkaev commented Mar 22, 2018 •

edited

Loading

codecov-io commented Mar 22, 2018 •

edited

Loading

kachkaev commented Mar 22, 2018 •

edited

Loading

robwise left a comment

robwise Mar 22, 2018

robwise Mar 22, 2018

robwise Mar 22, 2018

robwise Mar 22, 2018

olsonpm commented Mar 22, 2018

kachkaev commented Mar 22, 2018 •

edited

Loading

olsonpm commented Mar 22, 2018 •

edited

Loading

kachkaev commented Mar 22, 2018 •

edited

Loading

olsonpm commented Mar 22, 2018

kachkaev commented Mar 22, 2018

kachkaev commented Mar 23, 2018 •

edited

Loading

robwise commented Mar 24, 2018

krugar commented Aug 22, 2018

robwise commented Aug 23, 2018

Fallback to global instance of prettier before falling back to a bundled one #397

Fallback to global instance of prettier before falling back to a bundled one #397

Conversation

kachkaev commented Mar 22, 2018 • edited Loading

codecov-io commented Mar 22, 2018 • edited Loading

Codecov Report

kachkaev commented Mar 22, 2018 • edited Loading

robwise left a comment

Choose a reason for hiding this comment

robwise Mar 22, 2018

Choose a reason for hiding this comment

robwise Mar 22, 2018

Choose a reason for hiding this comment

robwise Mar 22, 2018

Choose a reason for hiding this comment

robwise Mar 22, 2018

Choose a reason for hiding this comment

olsonpm commented Mar 22, 2018

kachkaev commented Mar 22, 2018 • edited Loading

olsonpm commented Mar 22, 2018 • edited Loading

kachkaev commented Mar 22, 2018 • edited Loading

olsonpm commented Mar 22, 2018

kachkaev commented Mar 22, 2018

kachkaev commented Mar 23, 2018 • edited Loading

robwise commented Mar 24, 2018

krugar commented Aug 22, 2018

robwise commented Aug 23, 2018

kachkaev commented Mar 22, 2018 •

edited

Loading

codecov-io commented Mar 22, 2018 •

edited

Loading

kachkaev commented Mar 22, 2018 •

edited

Loading

kachkaev commented Mar 22, 2018 •

edited

Loading

olsonpm commented Mar 22, 2018 •

edited

Loading

kachkaev commented Mar 22, 2018 •

edited

Loading

kachkaev commented Mar 23, 2018 •

edited

Loading