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

Allow global installed configuration #3642

Merged
merged 2 commits into from Oct 22, 2018

Conversation

5 participants
@ybiquitous
Member

ybiquitous commented Sep 7, 2018

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

Fixes #1973

Is there anything in the PR that needs further explanation?

This patch fixes the following situation.

$ npm ls -g --depth 0 | grep stylelint
├── stylelint@9.5.0
├── stylelint-config-standard@18.2.0

$ cat ~/.stylelintrc.js
module.exports = {
  extends: ['stylelint-config-standard'],
};

$ stylelint ~/tmp/*.css
Error: Could not find "stylelint-config-standard". Do you need a `configBasedir`?
    at module.exports (/usr/local/lib/node_modules/stylelint/lib/utils/configurationError.js:8:28)
    at module.exports (/usr/local/lib/node_modules/stylelint/lib/utils/getModulePath.js:18:11)
    at loadExtendedConfig (/usr/local/lib/node_modules/stylelint/lib/augmentConfig.js:186:22)
    at resultPromise.then.resultConfig (/usr/local/lib/node_modules/stylelint/lib/augmentConfig.js:161:16)

This patch is inspired by this code in ESLint:

https://github.com/eslint/eslint/blob/c5b688e5350d16b330222f13749ab8b6ead517f8/lib/config/config-file.js#L333

If stylelint is globally installed to /usr/local/lib/node_modules/stylelint, then global node_modules directory should be /usr/local/lib/node_modules.

In this case, path.resolve(__dirname, "../../../../node_modules/") in lib/utils/getModulePath.js should evaluate to /usr/local/lib/node_modules:

/usr/local/lib/node_modules/stylelint/lib/utils/getModulePath.js
/usr/local/lib/node_modules/stylelint/lib/utils/     # __dirname
/usr/local/lib/node_modules/stylelint/lib/           # ../
/usr/local/lib/node_modules/stylelint/               # ../../
/usr/local/lib/node_modules/                         # ../../../
/usr/local/lib/                                      # ../../../../
/usr/local/lib/node_modules/                         # ../../../../node_modules/

NOTE: About testing

I can't find a way to test this patch. 😞
Please tell me if there is any good way. 🙇

@@ -2,18 +2,25 @@
"use strict";
const configurationError = require("./configurationError");
const resolve = require("path").resolve;

This comment has been minimized.

@ybiquitous

ybiquitous Sep 7, 2018

Member

To avoid shadowing local variable path.

module.exports = function(
const GLOBAL_NODE_MODULES = resolve(__dirname, "../../../../node_modules/");
module.exports = function getModulePath(

This comment has been minimized.

@ybiquitous

ybiquitous Sep 7, 2018

Member

Avoid anonymous function for better debugging.

@ybiquitous

This comment has been minimized.

Member

ybiquitous commented Sep 7, 2018

Perhaps I may have no permission to retry AppVeyor build... 🤔

image

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Sep 7, 2018

@ybiquitous when you log in with Github select “stylelint” account, not your personal.

@ybiquitous

This comment has been minimized.

Member

ybiquitous commented Sep 9, 2018

@hudochenkov I can't find the dropdown to switch to "stylelint" account... 😓

My login page: (https://ci.appveyor.com/login)
image

Is there some documents I must read to use AppVeyor with "stylelint" account?

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Sep 9, 2018

When I click Github on your screen, then it asks for which account login. I can't find anything related in stylelint organization configuration on Github.

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Sep 9, 2018

I'll restart job.

@ybiquitous

This comment has been minimized.

Member

ybiquitous commented Sep 20, 2018

@stylelint/core

What do you think about this proposal by @toptalo ?

I think it will be better to use global-modules because this path is related to project root and OS

It seems nice to me, but is there any project's policy about adding a new dependency?

ref: https://www.npmjs.com/package/global-modules

@ybiquitous ybiquitous force-pushed the ybiquitous:allow-globally-installed-config branch from 60bf3b3 to 2545144 Sep 20, 2018

@ybiquitous

This comment has been minimized.

Member

ybiquitous commented Sep 20, 2018

Experiment

When I examined the global-modules package, this package consider the Windows environment.

https://github.com/jonschlinkert/global-modules/blob/748f933b3e2a79f3c996f97d59e14a3571af9e10/index.js#L15

Extract the code:

function getPath() {
  if (isWindows()) {
    return path.resolve(prefix, 'node_modules');
  }
  return path.resolve(prefix, 'lib/node_modules');
}

So, I tried actually this PR's patch on Windows.
The npm i -g stlyelint command usually installs to the below folder (assume that the login username is IEUser):

C:\Users\IEUser\AppData\Roaming\npm\node_modules\stylelint

On such Windows environment, I verified that this patch's algorithm to search the global folder worked well:

C:\Users\IEUser\AppData\Roaming\npm\node_modules\stylelint\lib\utils\getModulePath.js
C:\Users\IEUser\AppData\Roaming\npm\node_modules\stylelint\lib\utils\     # __dirname
C:\Users\IEUser\AppData\Roaming\npm\node_modules\stylelint\lib\           # ..\
C:\Users\IEUser\AppData\Roaming\npm\node_modules\stylelint\               # ..\..\
C:\Users\IEUser\AppData\Roaming\npm\node_modules\                         # ..\..\..\
C:\Users\IEUser\AppData\Roaming\npm\                                      # ..\..\..\..\
C:\Users\IEUser\AppData\Roaming\npm\node_modules\                         # ..\..\..\..\node_modules\

image

Conclusion

We DON'T need to install the global-modules package.

What do you think about this, @toptalo?
(Sorry for erasing your comment to the past commit by my git rebase 🙇 )

@ybiquitous ybiquitous force-pushed the ybiquitous:allow-globally-installed-config branch from 2545144 to 5754213 Oct 3, 2018

@ybiquitous

This comment has been minimized.

Member

ybiquitous commented Oct 3, 2018

I do git rebase master.

const resolveFrom = require("resolve-from");
module.exports = function(
const GLOBAL_NODE_MODULES = resolve(__dirname, "../../../../node_modules/");

This comment has been minimized.

@ntwb

ntwb Oct 4, 2018

Member

Not everyones global node_modules folder will be located at ../../../../node_modules/

Edit: I made this comment before reading your existing comments on the PR @ybiquitous, I think we should add some tests for this, I'll try and think of how we can do that in the next few days

This comment has been minimized.

@ybiquitous

ybiquitous Oct 4, 2018

Member

Umm... what case can you assume?

This comment has been minimized.

@byCedric

byCedric Oct 12, 2018

I'm just casually dropping this here, do ignore if it's irrelevant! I see resolve-from package is used already, how about also using resolve-global? It has a fairly different approach compared to global-modules, created by the same developer as resolve-from, with Window's Appdata and yarn support.

This comment has been minimized.

@ybiquitous

ybiquitous Oct 12, 2018

Member

@byCedric Thanks! resolve-global seems a nice tool to me. 👍

But it doesn't support a Homebrew installed Node.js on macOS, so we may need both ways relative path and resolve-global.

For example, resolve-global with Homebrew installed Node.js:
( global-dirs is internally used in resolve-global )

const globalDirs = require("global-dirs")
console.log(globalDirs.npm.packages)
//=> /usr/local/Cellar/node/10.11.0/lib/node_modules

Our solution may be the following:

diff --git a/lib/utils/getModulePath.js b/lib/utils/getModulePath.js
index f71434cf..c808fb45 100644
--- a/lib/utils/getModulePath.js
+++ b/lib/utils/getModulePath.js
@@ -4,6 +4,7 @@
 const configurationError = require("./configurationError");
 const resolve = require("path").resolve;
 const resolveFrom = require("resolve-from");
+const resolveGlobal = require("resolve-global");
 
 const GLOBAL_NODE_MODULES = resolve(__dirname, "../../../../node_modules/");
 
@@ -18,6 +19,9 @@ module.exports = function getModulePath(
   if (!path) {
     path = resolveFrom.silent(process.cwd(), lookup);
   }
+  if (!path) {
+    path = resolveGlobal.silent(lookup);
+  }
   if (!path) {
     path = resolveFrom.silent(GLOBAL_NODE_MODULES, lookup);
   }

This comment has been minimized.

@ybiquitous

ybiquitous Oct 12, 2018

Member

Now I found global-modules. It uses internally global-prefix and it's search-path algorithm seems more safe to me.

This may make the relative-path approach unnecessary, so I'll try it. 💪

Please give me any feedback if you have any time. 🙏

E.g.

diff --git a/lib/utils/getModulePath.js b/lib/utils/getModulePath.js
index f71434cf..1bcad971 100644
--- a/lib/utils/getModulePath.js
+++ b/lib/utils/getModulePath.js
@@ -2,10 +2,8 @@
 "use strict";
 
 const configurationError = require("./configurationError");
-const resolve = require("path").resolve;
 const resolveFrom = require("resolve-from");
-
-const GLOBAL_NODE_MODULES = resolve(__dirname, "../../../../node_modules/");
+const globalModules = require("global-modules")
 
 module.exports = function getModulePath(
   basedir /*: string*/,
@@ -19,7 +17,7 @@ module.exports = function getModulePath(
     path = resolveFrom.silent(process.cwd(), lookup);
   }
   if (!path) {
-    path = resolveFrom.silent(GLOBAL_NODE_MODULES, lookup);
+    path = resolveFrom.silent(globalModules, lookup);
   }
   if (!path) {
     throw configurationError(

This comment has been minimized.

@ybiquitous

ybiquitous Oct 12, 2018

Member

Now global-modules has 191 dependents!

image

This comment has been minimized.

@ybiquitous

ybiquitous Oct 12, 2018

Member

I pushed 5c1397d and rebased the master branch.

ybiquitous added some commits Sep 7, 2018

@ybiquitous ybiquitous force-pushed the ybiquitous:allow-globally-installed-config branch from 5754213 to 5c1397d Oct 12, 2018

@ybiquitous

This comment has been minimized.

Member

ybiquitous commented Oct 12, 2018

Now I changed to a new way using global-modules. Please give me any feedback! ☺️

For details about this change, please see #3642 (comment).


We can try stylelint of this PR version, please follow this step (shell script):

# Create working directory
rm -rf stylelint-global-example
mkdir stylelint-global-example
cd stylelint-global-example

# You can choose your favorite Node.js manager instead of nvm
nvm install 10
nvm use 10

# Install stylelint and stylelint-config-standard
npm install -g ybiquitous/stylelint#allow-globally-installed-config stylelint-config-standard
npm ls -g --depth=0

# Configure
echo '{"extends":"stylelint-config-standard"}' > .stylelintrc.json

# Pass
echo 'a{}' > a.css
stylelint *.css
#=> a.css
#=>  1:2  ✖  Unexpected empty block   block-no-empty

# Fail
npm i -g stylelint
stylelint *.css
#=> Error: Could not find "stylelint-config-standard". Do you need a `configBasedir`?
@jeddy3

This comment has been minimized.

Member

jeddy3 commented Oct 15, 2018

@ybiquitous Thanks for your continued work on this! Does anyone see any potential pitfalls in merging this, otherwise I think we can get it in?

@jeddy3

jeddy3 approved these changes Oct 17, 2018

LGTM

@jeddy3 jeddy3 referenced this pull request Oct 17, 2018

Closed

Release 9.7.0 #3726

4 of 4 tasks complete
@ntwb

ntwb approved these changes Oct 21, 2018

@ntwb

This comment has been minimized.

Member

ntwb commented Oct 21, 2018

I’d love to see some tests for this in the future, I’m also not sure how to do that now though

@ybiquitous

This comment has been minimized.

Member

ybiquitous commented Oct 22, 2018

I’d love to see some tests for this in the future, I’m also not sure how to do that now though

Yes, I agree 👍
We may be able to test by Docker, but I'm worrying that tests time may become slow... 🤔
(Of course, there may be other better ways)

@jeddy3 jeddy3 merged commit d080a87 into stylelint:master Oct 22, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 96.384%
Details
@jeddy3

This comment has been minimized.

Member

jeddy3 commented Oct 22, 2018

  • Added: allow globally installed configuration (#3642).

ybiquitous added a commit to stylelint/stylelint-config-standard that referenced this pull request Nov 12, 2018

ybiquitous added a commit to stylelint/stylelint-config-recommended that referenced this pull request Nov 12, 2018

ntwb added a commit to stylelint/stylelint-config-standard that referenced this pull request Nov 13, 2018

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