Added eslint-plugin-react to the CLIEngine #88

muuvmuuv · 2018-08-28T16:22:58Z

See: #87

Glavin001

Looking good so far!

Left to-do:

Add JSX language (I think??)
Add at least one test to https://github.com/Unibeautify/beautifier-eslint/tree/master/test

Glavin001 · 2018-08-28T20:23:37Z

src/index.ts

@@ -23,6 +23,11 @@ export const beautifier: Beautifier = {
      name: "ESLint",
      package: "eslint",
    },
+    {
+      type: DependencyType.Node,
+      name: "ESLintPluginReact",


ESLint-plugin-React (as seen in https://www.npmjs.com/package/eslint-plugin-react ) may be a better name.

Good approach, but what about on L60, where we assign it to a variable. There I need to call it ESLintPluginReact and then we would have two different styles.

Yup, change line 60, too, so they are the same 👍 .

muuvmuuv · 2018-08-29T08:16:27Z

@Glavin001 I added JSX like that: JSX: options.JavaScript,

Now I got stuck by testing it. I had to rename the .eslintrc.yml to .json because I couldn't get it running in yml. I really tried adapting the react rules but had no luck. I think the problem below is caused by jest or some directory issue where eslint is not looking in the test rather then in the root of the project for a configuration file.

https://gist.github.com/muuvmuuv/eae245f991d0c6b6e012a00326ee5598

Glavin001 · 2018-08-29T12:08:50Z

@szeck87 any advice on @muuvmuuv 's problem above in #88 (comment) ? I think you setup most of this beautifier.

Glavin001 · 2018-08-29T12:10:34Z

@muuvmuuv I see your Gist with the file changes and test report. You can commit your changes to this PR, so we can see what changes you've done, let Travis CI run the tests, and comment on lines of code 👍 .

stevenzeck · 2018-08-29T14:55:24Z

@muuvmuuv you need to include the filePath when calling the beautify method, otherwise the beautifier won't know where to look for the config file. Adding that allows it to find the config file.

From there ESLint and/or the React plugin errors due to this message: Parsing error: The keyword 'const' is reserved. So it doesn't return an output in the result. I think you need to have "ecmaVersion": 6, in your ESLint config under parserOptions.

stevenzeck

The core ESLint beautifier should continue to work if the user is not using the React plugin.

Glavin001 · 2018-08-29T17:01:13Z

package.json

@@ -37,7 +37,8 @@
    "@types/jest": "23.3.1",
    "@types/node": "10.9.3",
    "@types/read-pkg-up": "3.0.1",
-    "eslint": "5.4.0",
+    "eslint": "^5.4.0",
+    "eslint-plugin-react": "^7.11.1",


@szeck87 pointed out to me on Slack eslint-plugin-react should also be added as a peerDependency:

beautifier-eslint/package.json

Lines 33 to 40 in 221ef55

"eslint": "^4.19.1 || ^5.0.0"

},

"devDependencies": {

"@types/eslint": "4.16.3",

"@types/jest": "23.3.1",

"@types/node": "10.9.3",

"@types/read-pkg-up": "3.0.1",

"eslint": "^5.4.0",

Glavin001 · 2018-08-29T17:02:42Z

src/index.ts

+      type: DependencyType.Node,
+      name: "ESLint-plugin-React",
+      package: "eslint-plugin-react",
+    },


Should be optional. cc @szeck87 from #87 (comment)

Yep, please add optional: true

Glavin001 · 2018-08-29T17:06:45Z

src/index.ts

@@ -51,6 +57,9 @@ export const beautifier: Beautifier = {
  }: BeautifierBeautifyData) {
    return new Promise<string>((resolve, reject) => {
      const { CLIEngine } = dependencies.get<NodeDependency>("ESLint").package;
+      const ESLintPluginReact = dependencies.get<NodeDependency>(


To allow for the React plugin to be optional, we should check beautifierConfig (or config after line 63).

Installation instructions from https://www.npmjs.com/package/eslint-plugin-react show:

"extends": [ "eslint:recommended", "plugin:react/recommended" ]

So we should check for plugin:react/ and if it exists in extends then get the React plugin dependency (line 60) and add the plugin (line 79).

cc @szeck87 for #87 (comment)

You can try that. If that can't be done, dependencies.get<NodeDependency>("ESLint-plugin-React") will have an error that you can check for.

@szeck87 Good point! Actually, let's just skip the checking of beautifierConfig entirely, that sounds like an unnecessary rabbit hole.

We can use the .has method! https://api.unibeautify.com/classes/_dependencymanager_dependencymanager_.dependencymanager.html#has

Like this:

if (dependencies.has("ESLint-plugin-React")) { cli.addPlugin("eslint-plugin-react", dependencies.get<NodeDependency>("ESLint-plugin-React").package); }

🎉

Whoops. @szeck87 pointed out .has will not work here.

We actually want to use dependency.isInstalled: https://api.unibeautify.com/classes/_dependencymanager_dependency_.dependency.html#isinstalled

So more like this:

const reactPlugin = dependencies.get<NodeDependency>("ESLint-plugin-React"); if (reactPlugin.isInstalled) { cli.addPlugin("eslint-plugin-react", reactPlugin.package); }

Feel free to clean up the above.

When I do this I'm getting : A Promise was found that appears to not have resolve or reject invoked on all code paths (promise-must-complete) @Glavin001

Yea I noticed that too, might have to comment on the line above the if statement:

// tslint:disable-next-line:promise-must-complete

Where is this error coming from @muuvmuuv ? Could you push a commit with this?

Maybe you already did push a commit. I currently see Travis CI failing:

npm ERR! Unexpected token } in JSON at position 994

Added "if plugin installed" to react plugin

# Conflicts: # package.json

stevenzeck · 2018-08-30T19:13:20Z

src/index.ts

+      type: DependencyType.Node,
+      name: "ESLint-plugin-React",
+      package: "eslint-plugin-react",
+    },


Yep, please add optional: true

stevenzeck · 2018-08-30T19:13:41Z

package.json

@@ -30,14 +30,16 @@
  "homepage": "https://github.com/Unibeautify/beautifier-eslint#readme",
  "peerDependencies": {
    "unibeautify": ">= 0.12.1",
-    "eslint": "^4.19.1 || ^5.0.0"
+    "eslint": "^4.19.1 || ^5.0.0",
+    "eslint-plugin-react": "^7.11.1",


Remove the comma at the end here

Glavin001 · 2018-08-30T20:52:23Z

src/index.ts

@@ -67,6 +76,10 @@ export const beautifier: Beautifier = {
        rules: finalOptions,
      };
      const cli: CLIEngine = new CLIEngine(cliOptions);
+      // tslint:disable-next-line:promise-must-complete


This may be a bug with https://github.com/Microsoft/tslint-microsoft-contrib/
If you can create a much smaller test case, you could report an issue 👍 .

It looks like any if is a branch, and doesn't bother looking what it actually does to consider whether the promise completes or not (since we are using return new Promise<string>((resolve, reject) => {).

Maybe it's safer to remove that block and change the returns at the bottom to return Promise.resolve.

Let's not make any major changes yet. We can try different approaches in a later PR 👍 .

@Glavin001 would you mind creating a small test case and make an issue there? I think you can way better explain that error case.

Glavin001

@szeck87 had some good comments. Once those are fixed up we can merge 🎉!

Removed trailing comma

stevenzeck · 2018-08-31T14:58:58Z

src/index.ts

@@ -21,11 +21,18 @@ export const beautifier: Beautifier = {
    {
      type: DependencyType.Node,
      name: "ESLint",
-      package: "eslint",
+      package: "eslint"


Put a comma at the end of package: "eslint"

Can we enable a linter for this? I really don't know when to put a comma and when I shouldn't …

https://palantir.github.io/tslint/rules/trailing-comma/ should be catching it, but it's not.

Looks like it is set to ignore for multiline: https://github.com/Unibeautify/beautifier-eslint/blob/master/tslint.json#L294-L300

@muuvmuuv Could you tweak this and make sure TSLint is telling you to add the comma? :)

stevenzeck · 2018-08-31T14:59:07Z

src/index.ts

+      type: DependencyType.Node,
+      name: "ESLint-plugin-React",
+      package: "eslint-plugin-react",
+      optional: true


Put a comma at the end of optional: true

stevenzeck · 2018-08-31T15:09:59Z

@muuvmuuv just add in those two commas I mentioned and I'll approve.

muuvmuuv · 2018-09-05T12:44:59Z

@szeck87 @Glavin001 I've now added this ESLint issue we discussed before. Sorry that the file is beautified that much, but that is what Unibeautify is doing when I save that file.

stevenzeck · 2018-09-05T14:17:10Z

@muuvmuuv can you rebase again so the CI tests can run?

Glavin001

I think this is good to go!

Glavin001 · 2018-09-06T02:32:23Z

Awesome work, @muuvmuuv ! Thanks for contributing! 🎉

Glavin001 · 2018-09-19T19:44:52Z

Published to v0.6.0! 🎉

Added eslint-plugin-react to the CLIEngine

ff3ab4e

Glavin001 requested review from stevenzeck and Glavin001 August 28, 2018 20:22

Glavin001 assigned Glavin001 and muuvmuuv Aug 28, 2018

Glavin001 added the add plugin Add ESLint plugin support label Aug 28, 2018

Glavin001 requested changes Aug 28, 2018

View reviewed changes

Added test and renamed plugin name as suggested in #88

135397e

Marvin Heilemann added 2 commits August 29, 2018 17:55

- Added filePath so the test will find a eslint config

7d785f4

Merge branch 'master' into master

221ef55

stevenzeck suggested changes Aug 29, 2018

View reviewed changes

Glavin001 requested changes Aug 29, 2018

View reviewed changes

Marvin Heilemann added 3 commits August 30, 2018 21:05

Added eslint-plugin-react as peerDependencie

af5f038

Added "if plugin installed" to react plugin

Merge branch 'master' of https://github.com/muuvmuuv/beautifier-eslint

df52510

# Conflicts: # package.json

Merge branch 'master' into master

e6c8f61

stevenzeck suggested changes Aug 30, 2018

View reviewed changes

Glavin001 reviewed Aug 30, 2018

View reviewed changes

Glavin001 requested changes Aug 30, 2018

View reviewed changes

Glavin001 and others added 4 commits August 30, 2018 17:54

Merge branch 'master' into master

4441f2c

Made eslint-plugin-react optional

b7ba901

Removed trailing comma

Merge branch 'master' of https://github.com/muuvmuuv/beautifier-eslint

46243fe

Merge branch 'master' into master

1a31cfb

stevenzeck suggested changes Aug 31, 2018

View reviewed changes

Added commas

2a1e656

Merge branch 'master' of https://github.com/muuvmuuv/beautifier-eslint

4ca650c

stevenzeck approved these changes Aug 31, 2018

View reviewed changes

tslint: trailing-comma multiline always

6fef048

Fixed test errors

e21d7b1

Merge branch 'master' into master

35868c4

Glavin001 mentioned this pull request Sep 6, 2018

Error: Failed to load plugin: xxx #87

Open

Glavin001 approved these changes Sep 6, 2018

View reviewed changes

Glavin001 merged commit 2ebbe9f into Unibeautify:master Sep 6, 2018

	"eslint": "^4.19.1 \|\| ^5.0.0"
	},
	"devDependencies": {
	"@types/eslint": "4.16.3",
	"@types/jest": "23.3.1",
	"@types/node": "10.9.3",
	"@types/read-pkg-up": "3.0.1",
	"eslint": "^5.4.0",

Added eslint-plugin-react to the CLIEngine #88

Added eslint-plugin-react to the CLIEngine #88

Conversation

muuvmuuv commented Aug 28, 2018

Glavin001 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

muuvmuuv commented Aug 29, 2018 • edited Loading

Glavin001 commented Aug 29, 2018

Glavin001 commented Aug 29, 2018

stevenzeck commented Aug 29, 2018 • edited Loading

stevenzeck 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

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

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

Glavin001 left a comment

Choose a reason for hiding this comment

stevenzeck Aug 31, 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

stevenzeck Aug 31, 2018 • edited Loading

Choose a reason for hiding this comment

stevenzeck commented Aug 31, 2018

muuvmuuv commented Sep 5, 2018

stevenzeck commented Sep 5, 2018

Glavin001 left a comment

Choose a reason for hiding this comment

Glavin001 commented Sep 6, 2018

Glavin001 commented Sep 19, 2018

muuvmuuv commented Aug 29, 2018 •

edited

Loading

stevenzeck commented Aug 29, 2018 •

edited

Loading

stevenzeck Aug 31, 2018 •

edited

Loading

stevenzeck Aug 31, 2018 •

edited

Loading