Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete Phenomic and update dependencies #74

Merged
merged 7 commits into from
Jul 14, 2019

Conversation

ybiquitous
Copy link
Member

@ybiquitous ybiquitous commented Jul 13, 2019

The migration to Docusaurus has worked (see #67), so this deletes Phenomic code as the old engine.

Which issue, if any, is this issue related to?

None, but for details see #67.

Is there anything in the PR that needs further explanation?

This PR does the followings:

  • Delete unused directories and files.
  • Update dependencies.
  • Use Node 12 (latest) in Travis CI.
  • Update README.

Demo: https://stylelint-docusaurus.netlify.com/

The migration to Docusaurus has worked (see stylelint#67), so this deletes Phenomic code as the old engine.

- Delete unused directories and files.
- Update dependencies.
- Use Node 12 (latest) in Travis CI.
Travis CI and Netlify recognize a `.nvmrc` file to detect needed Node version.
Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup!

There are few thing to improve developer experience.

Also, I noticed following warning after linting:

Warning: React version not specified in eslint-plugin-react settings. See https://github.com/yannickcr/eslint-plugin-react#configuration .

package.json Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@ybiquitous
Copy link
Member Author

@hudochenkov Thanks for the review! I pushed the changes you reviewed. 😃


About the following comment,

Also, I noticed following warning after linting:

Warning: React version not specified in eslint-plugin-react settings. See https://github.com/yannickcr/eslint-plugin-react#configuration .

The warning can be fixed by the following change: ⬇️

diff --git a/package.json b/package.json
index ca8119b..401ccc5 100644
--- a/package.json
+++ b/package.json
@@ -23,6 +23,11 @@
     "start": "npm run gendoc && cd website && npm start"
   },
   "eslintConfig": {
+    "settings": {
+      "react": {
+        "version": "16.5"
+      }
+    },
     "root": true,
     "extends": [
       "eslint:recommended",

However, I'm thinking about the description on eslint-plugin-react README:

React version. "detect" automatically picks the version you have installed.
You can also use 16.0, 16.3, etc, if you want to override the detected value.
default to latest and warns if missing
It will default to "detect" in the future

The used React version in this project depends on Docusaurus, and we don't need to define the React version explicitly. So, if the React depended by Docusaurus would be updated, we might need to also change the ESLint settings (eslintConfig.settings.react.version).

I don't think a good idea to depend on a specific React version implicitly, and I think better to ignore the warning now. Because the warning may be fixed naturally in the future. What do you think about my opinion?

@hudochenkov
Copy link
Member

Thank you for changes!

I think we need to explicitly set "version": "detect" for React plugin at the moment, if I understood correctly. Because “It will default to "detect" in the future”, but not now.

I also noticed this PostCSS warning:

> @ build /home/travis/build/stylelint/stylelint.io/website
> docusaurus-build
generate.js triggered...
sitemap.js triggered...
Without from option PostCSS could generate wrong source map and will not find Browserslist config. Set it to CSS file path or to undefined to prevent this warning.
Site built successfully. Generated files in 'build' folder.

I can't find any issue report in Docusaurus repository about it. Is is something we could fix?

@ybiquitous
Copy link
Member Author

I think we need to explicitly set "version": "detect" for React plugin at the moment, if I understood correctly. Because “It will default to "detect" in the future”, but not now.

Make sense. 👍
But when setting version: "detect", the following error raises... 😢

$ npm run lint:js

> stylelint-io@ lint:js /Users/koba/git/ybiquitous/stylelint.io
> eslint . --ignore-path .gitignore

Warning: React version was set to "detect" in eslint-plugin-react settings, but the "react" package is not installed. Assuming latest React version for linting.
Error: Error while loading rule 'react/no-deprecated': Cannot find module 'react' from '/Users/koba/git/ybiquitous/stylelint.io'
Occurred while linting /Users/koba/git/ybiquitous/stylelint.io/scripts/generate-stylelint-docs.js
    at Function.module.exports [as sync] (/Users/koba/git/ybiquitous/stylelint.io/node_modules/resolve/lib/sync.js:71:15)
    at detectReactVersion (/Users/koba/git/ybiquitous/stylelint.io/node_modules/eslint-plugin-react/lib/util/version.js:19:31)
    at getReactVersionFromContext (/Users/koba/git/ybiquitous/stylelint.io/node_modules/eslint-plugin-react/lib/util/version.js:39:25)
    at Object.testReactVersion (/Users/koba/git/ybiquitous/stylelint.io/node_modules/eslint-plugin-react/lib/util/version.js:107:35)
    at usedPropTypesInstructions (/Users/koba/git/ybiquitous/stylelint.io/node_modules/eslint-plugin-react/lib/util/usedPropTypes.js:271:48)
    at Function.componentRule (/Users/koba/git/ybiquitous/stylelint.io/node_modules/eslint-plugin-react/lib/util/Components.js:763:37)
    at createRuleListeners (/Users/koba/git/ybiquitous/stylelint.io/node_modules/eslint/lib/linter/linter.js:697:21)
    at Object.keys.forEach.ruleId (/Users/koba/git/ybiquitous/stylelint.io/node_modules/eslint/lib/linter/linter.js:865:31)
    at Array.forEach (<anonymous>)
    at runRules (/Users/koba/git/ybiquitous/stylelint.io/node_modules/eslint/lib/linter/linter.js:810:34)
npm ERR! code ELIFECYCLE
npm ERR! errno 2
npm ERR! stylelint-io@ lint:js: `eslint . --ignore-path .gitignore`
npm ERR! Exit status 2
diff --git a/package.json b/package.json
index ca8119b..02fad9a 100644
--- a/package.json
+++ b/package.json
@@ -28,6 +28,11 @@
       "eslint:recommended",
       "plugin:react/recommended"
     ],
+    "settings": {
+      "react": {
+        "version": "detect"
+      }
+    },
     "parserOptions": {
       "ecmaVersion": "2019"
     },

Do you have any good idea?

@hudochenkov
Copy link
Member

Looks like a known issue jsx-eslint/eslint-plugin-react#2218.

Let's use version: "16.8" as the the current latest version and version which is installed with Docusaurus. On the other hand:

I don't think a good idea to depend on a specific React version implicitly, and I think better to ignore the warning now. Because the warning may be fixed naturally in the future. What do you think about my opinion?

I understand your point. Warnings are telling that something wrong, and I don't like when something wrong :) We have only two ways to remove warning: specify version or remove React plugin.

@ybiquitous
Copy link
Member Author

Yes, there are some pros and cons, but it seems good to set version: "16.8". 👍

> Warning: React version not specified in eslint-plugin-react settings. See https://github.com/yannickcr/eslint-plugin-react#configuration .
@ybiquitous
Copy link
Member Author

Without from option PostCSS could generate wrong source map and will not find Browserslist config. Set it to CSS file path or to undefined to prevent this warning.

The above PostCSS warning seems to be due to cssnano call in Docusaurus code. See below:

https://github.com/facebook/docusaurus/blob/dc15a51745212469ce2beede6f293e4317df7e9a/packages/docusaurus-1.x/lib/server/utils.js#L52-L56

On the other hand, from: undefined option is set for autoprefixer:

https://github.com/facebook/docusaurus/blob/dc15a51745212469ce2beede6f293e4317df7e9a/packages/docusaurus-1.x/lib/server/utils.js#L61-L64

Perhaps, PostCSS seems to warn here:

https://github.com/postcss/postcss/blob/fbcb35aece6b16f75e52a1e0c00c183404f68c63/lib/lazy-result.es6#L192-L200

Thus, I think good to set NODE_ENV=production in order to prevent the warning. For example:

diff --git a/package.json b/package.json
index e1f03f8..2344406 100644
--- a/package.json
+++ b/package.json
@@ -19,7 +19,7 @@
     "test": "npm run build",
     "pregendoc": "rimraf docs",
     "gendoc": "node ./scripts/generate-stylelint-docs docs",
-    "build": "npm run gendoc && cd website && npm run build",
+    "build": "npm run gendoc && cd website && NODE_ENV=production npm run build",
     "start": "npm run gendoc && cd website && npm start"
   },
   "eslintConfig": {

But if using the solution, should we think to install cross-env for Windows users?

@hudochenkov
Copy link
Member

hudochenkov commented Jul 14, 2019

Good investigations!

But if using the solution, should we think to install cross-env for Windows users?

I don't think we need it at the moment, because I don't know any contributor to the website who is using Windows.

@ybiquitous
Copy link
Member Author

OK, I will push the change of build script.

Or Docusaurus v2 seems to set NODE_ENV=production internally, so I can also send a patch to Docusaurus v1:

https://github.com/facebook/docusaurus/blob/dc15a51745212469ce2beede6f293e4317df7e9a/packages/docusaurus/src/commands/build.ts#L51

Anyway, NODE_ENV=production is necessary for the PR.

> Without `from` option PostCSS could generate wrong source map and will not find Browserslist config. Set it to CSS file path or to `undefined` to prevent this warning.
Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing!

@hudochenkov hudochenkov merged commit 755681e into stylelint:master Jul 14, 2019
@ybiquitous ybiquitous deleted the delete-phenomic branch July 14, 2019 15:27
@ntwb
Copy link
Member

ntwb commented Jul 14, 2019

Awesome, thanks a bunch @ybiquitous

@ybiquitous
Copy link
Member Author

OK, I will push the change of build script.

Or Docusaurus v2 seems to set NODE_ENV=production internally, so I can also send a patch to Docusaurus v1:

https://github.com/facebook/docusaurus/blob/dc15a51745212469ce2beede6f293e4317df7e9a/packages/docusaurus/src/commands/build.ts#L51

Anyway, NODE_ENV=production is necessary for the PR.

I opened the PR facebook/docusaurus#1664 to fix the warning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants