feat: use "prettier-tslint" and "prettier-eslint" when they exist #54

aleclarson · 2018-12-13T18:18:29Z

When the scm root has ./node_modules/prettier-tslint and/or ./node_modules/prettier-eslint, we use them on any applicable modules.

We prefer prettier-eslint for .js, .jsx, and .mjs modules. If prettier-eslint does not exist, we try prettier-tslint and then prettier alone.

We prefer prettier-tslint for .ts and .tsx. If prettier-tslint does not exist, we try prettier-eslint and then prettier alone.

All other file types use prettier only.

aleclarson · 2018-12-13T18:18:51Z

PS: You can add me as a maintainer here, too. ;)

azz · 2018-12-14T05:48:23Z

src/formatFiles.js

+function loadFormat(rootDirectory, formatName, exportName) {
+  let entry;
+  try {
+    entry = require.resolve(join(rootDirectory, 'node_modules', formatName));


This won't work with yarn PnP.

Are you saying it breaks pretty-quick for Yarn PnP installs, or just that prettier-eslint and prettier-tslint won't be found?

Have you actually tested this with Yarn PnP?

The latter. Projects using PnP don't have node_modules

Okay, that's fine. We can patch PnP support for this feature in a later patch.

azz · 2018-12-16T10:17:52Z

Thinking I'd favour a more explicit API, like pretty-quick --tslint, pretty-quick --eslint. It will save a small amount of IO for users who don't use those tools, and users who have e.g. prettier-eslint still in node_modules (maybe unused) will be less confused when the behaviour changes after upgrading this package.

We can also make it work with PnP that way.

aleclarson · 2018-12-16T15:38:13Z

We can also make it work with PnP that way.

Why would replacing implicit loading with an explicit flag make PnP work? We already wrap the require.resolve call in a try..catch block, so PnP shouldn't choke on it.

azz · 2018-12-17T08:18:57Z

We can just do if (opts.eslint) require('prettier-eslint'), shouldn't need to hardcode node_modules.

aleclarson · 2018-12-17T15:13:35Z

Can you explain why someone would want pretty-quick to not use prettier-eslint and/or prettier-tslint when they are installed?

We can remove the hard-coded node_modules, but I don't get why adding new flags is ideal.

aleclarson · 2018-12-17T15:50:55Z

I've improved the implicit behavior by ensuring the resolved prettier-tslint and/or prettier-eslint paths are never globally-installed versions. Basically, they must be in your package.json to signal to pretty-quick that you're using them.

src/formatFiles.js

azz · 2018-12-17T23:02:22Z

src/formatFiles.js

-    );
+
+    const format =
+      (/\.(mjs|jsx?)$/.test(file) && (eslintFormat || tslintFormat)) ||


This won't work for files with shebangs (which prettier supports). Maybe using prettier.getFileInfo would be better?

You're talking about executable files with no file extension that contain Javascript and begin with a shebang, yes?

Absolutely.

prettier/prettier#5149

Let's support shebangs in a later patch. No biggie if they don't get linted for now.

When the scm root has "./node_modules/prettier-tslint" and/or "./node_modules/prettier-eslint", we use them on applicable modules.

So we can use the `getFileInfo` export.

aleclarson · 2018-12-23T15:48:48Z

I've added shebang support (using prettier.getFileInfo), which requires Prettier v1.13.0 or higher.
I've updated the peer/dev dependencies on prettier to match.

Like I said, we can add Yarn PnP support in the future. I think we would take this approach. Specifically, importing pnpapi to determine if a repository is using PnP, and then calling pnp.resolveRequest.

LGTM! :)

aleclarson · 2018-12-23T16:09:04Z

Hmm.. Looks like Travis is using a version of Prettier that doesn't include getFileInfo.

Even though I updated yarn.lock. 🤔

@azz Could you take a look?

azz

Sorry I haven't had a chance to look at the build issue. Christmas is a busy time! Left a few comments

package.json

azz · 2018-12-27T11:14:14Z

src/formatFiles.js

+      format === prettierFormat
+        ? format(input, ((options.filepath = file), options))
+        : format({
+            text: input,


Could you add a test for this code path?

I've tested locally (and you can too) with:

git clone -b feat1-test https://github.com/aleclarson/pretty-quick cd pretty-quick cat README.md

I'll try to get around to writing a test when I can.

azz · 2018-12-27T11:15:08Z

src/formatFiles.js

+
+    const parser = getFileInfo.sync(file).inferredParser;
+    const format =
+      (parser === 'babylon' && (eslintFormat || tslintFormat)) ||


Does eslint support flow? edit: Nope!

babylon supports flow language too. You can write JavaScript and still use the flow parser.

Okay, but passing Flow-typed JavaScript into eslint does not work.

Are you saying there's a chance that file will contain Flow-typed JavaScript when parser === 'babylon'?

azz · 2018-12-27T11:19:05Z

I deleted the build caches and restarted the build. Hopefully that helps.

- rename "isSupportedExtension" function to "isFileSupported" - use `prettier.getFileInfo` to detect shebangs - upgrade prettier to 1.15.0+ (where shebang support was added)

aleclarson · 2018-12-27T19:45:51Z

Once tests are added, this is ready to merge.

Things to fix later:

add Yarn PnP support
get prettier-eslint to work with shebang modules

MichaelDeBoey · 2019-10-14T06:34:27Z

Any news on this one? 🤔

Ping @aleclarson @azz

JounQin · 2024-01-16T10:15:30Z

No thanks, but you can looking forward to https://github.com/prettier-eslint/prettier-plugin.

nit: clearer variable name

3ae5e08

aleclarson force-pushed the feat1 branch from bf23685 to 132b173 Compare December 13, 2018 18:28

azz reviewed Dec 14, 2018

View reviewed changes

azz reviewed Dec 17, 2018

View reviewed changes

feat: use "prettier-tslint" and "prettier-eslint" when they exist

9ec3d60

When the scm root has "./node_modules/prettier-tslint" and/or "./node_modules/prettier-eslint", we use them on applicable modules.

aleclarson force-pushed the feat1 branch from c96f513 to 9ec3d60 Compare December 23, 2018 15:32

feat: require prettier 1.13.0+

ab98aeb

So we can use the `getFileInfo` export.

fix: use getFileInfo to infer eslint/tslint

299ddd2

aleclarson force-pushed the feat1 branch from 455f58c to 2b85378 Compare December 23, 2018 16:02

azz requested changes Dec 27, 2018

View reviewed changes

aleclarson added 2 commits December 27, 2018 10:43

chore: upgrade to jest@^23.6.0

f1d88ae

feat: shebang support

829524a

- rename "isSupportedExtension" function to "isFileSupported" - use `prettier.getFileInfo` to detect shebangs - upgrade prettier to 1.15.0+ (where shebang support was added)

aleclarson force-pushed the feat1 branch from 2b85378 to 829524a Compare December 27, 2018 18:15

aleclarson mentioned this pull request Dec 29, 2018

use pretty-quick wclr/yalc#59

Merged

JounQin closed this Jan 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use "prettier-tslint" and "prettier-eslint" when they exist #54

feat: use "prettier-tslint" and "prettier-eslint" when they exist #54

aleclarson commented Dec 13, 2018

aleclarson commented Dec 13, 2018

azz Dec 14, 2018

aleclarson Dec 14, 2018

aleclarson Dec 14, 2018

azz Dec 14, 2018

aleclarson Dec 15, 2018

azz commented Dec 16, 2018

aleclarson commented Dec 16, 2018

azz commented Dec 17, 2018 •

edited

Loading

aleclarson commented Dec 17, 2018 •

edited

Loading

aleclarson commented Dec 17, 2018 •

edited

Loading

azz Dec 17, 2018

aleclarson Dec 17, 2018 •

edited

Loading

azz Dec 18, 2018

azz Dec 18, 2018

aleclarson Dec 22, 2018

aleclarson commented Dec 23, 2018 •

edited

Loading

aleclarson commented Dec 23, 2018

azz left a comment •

edited

Loading

azz Dec 27, 2018

aleclarson Dec 27, 2018 •

edited

Loading

azz Dec 27, 2018

aleclarson Dec 27, 2018 •

edited

Loading

azz Dec 28, 2018

aleclarson Dec 28, 2018

aleclarson Dec 28, 2018

azz commented Dec 27, 2018

aleclarson commented Dec 27, 2018 •

edited

Loading

MichaelDeBoey commented Oct 14, 2019

JounQin commented Jan 16, 2024

feat: use "prettier-tslint" and "prettier-eslint" when they exist #54

feat: use "prettier-tslint" and "prettier-eslint" when they exist #54

Conversation

aleclarson commented Dec 13, 2018

aleclarson commented Dec 13, 2018

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

azz commented Dec 16, 2018

aleclarson commented Dec 16, 2018

azz commented Dec 17, 2018 • edited Loading

aleclarson commented Dec 17, 2018 • edited Loading

aleclarson commented Dec 17, 2018 • edited Loading

Choose a reason for hiding this comment

aleclarson Dec 17, 2018 • edited Loading

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

aleclarson commented Dec 23, 2018 • edited Loading

aleclarson commented Dec 23, 2018

azz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleclarson Dec 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleclarson Dec 27, 2018 • edited Loading

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

azz commented Dec 27, 2018

aleclarson commented Dec 27, 2018 • edited Loading

MichaelDeBoey commented Oct 14, 2019

JounQin commented Jan 16, 2024

azz commented Dec 17, 2018 •

edited

Loading

aleclarson commented Dec 17, 2018 •

edited

Loading

aleclarson commented Dec 17, 2018 •

edited

Loading

aleclarson Dec 17, 2018 •

edited

Loading

aleclarson commented Dec 23, 2018 •

edited

Loading

azz left a comment •

edited

Loading

aleclarson Dec 27, 2018 •

edited

Loading

aleclarson Dec 27, 2018 •

edited

Loading

aleclarson commented Dec 27, 2018 •

edited

Loading