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

`ignore` dependency upgrade should be reverted #41

Open
kgryte opened this issue Sep 23, 2019 · 31 comments
Open

`ignore` dependency upgrade should be reverted #41

kgryte opened this issue Sep 23, 2019 · 31 comments

Comments

@kgryte
Copy link

@kgryte kgryte commented Sep 23, 2019

Subject of the issue

In the 7.0.0 release, the ignore dependency was upgraded to version 5, a semver major upgrade, which changed how relative paths are ignored (see kaelzhang/node-ignore#20).

At L38 of the ignore.js file, you resolve a file path to a relative path. This causes ignore to throw an error because ignore does not accept relative paths.

For example, when running remark on a Markdown file, I encounter the following error:

RangeError: path should be a `path.relative()`d string, but got "../../path/to/file.md"

Your environment

  • OS: MacOS
  • Packages: unified-engine@7.0.0
  • Env: Node.js@12.9.1, npm@6.10.2

Steps to reproduce

Tell us how to reproduce this issue. Please provide a working and simplified example.

Run a command similar to the following (using remark and not unified-engine directly):

$ ./node_modules/.bin/remark --rc-path=./path/to/.remarkrc.js --ignore-path=./path/to/.remarkignore --no-ignore --no-config ./path/to/file.md

For the used .remarkignore file, see here.

Expected behaviour

No error should occur.

Actual behaviour

The following error is encountered:

RangeError: path should be a `path.relative()`d string, but got "../../path/to/file.md"
@wooorm

This comment has been minimized.

Copy link
Member

@wooorm wooorm commented Sep 23, 2019

Thanks for the detailed report, do you really think we should revert, or can we fix this?

@kgryte

This comment has been minimized.

Copy link
Author

@kgryte kgryte commented Sep 23, 2019

Not clear to me how it would be fixed. Maybe you have an idea?

@kgryte

This comment has been minimized.

Copy link
Author

@kgryte kgryte commented Sep 23, 2019

According to the issue thread, resolving the correct ignore path should be done in userland, but not clear to me what your intent is in the relevant unified-engine code.

@kgryte

This comment has been minimized.

Copy link
Author

@kgryte kgryte commented Sep 23, 2019

I recommended reverting as the bug seems directly related to the change in ignore dependency behavior.

@wooorm

This comment has been minimized.

Copy link
Member

@wooorm wooorm commented Sep 23, 2019

I’d rather include a fix, because that way we can keep up with the latest ignore.

What I’m wondering is whether we could pass an absolute path in instead, as a fix?

@kgryte

This comment has been minimized.

Copy link
Author

@kgryte kgryte commented Sep 23, 2019

No. Already tried that. Does not work.

@kgryte

This comment has been minimized.

Copy link
Author

@kgryte kgryte commented Sep 23, 2019

Not clear to me that you need to keep up with the latest ignore, unless the previous behavior was knowingly buggy and/or deficient.

@kgryte

This comment has been minimized.

Copy link
Author

@kgryte kgryte commented Sep 23, 2019

Here is the error I got when I changed the relevant line to use an absolute path:

callback(null, normal ? ignore.ignores(resolve(self.cwd, filePath)) : false)
RangeError: path should be a `path.relative()`d string, but got "/absolute/file/path/to/file.md"
@wooorm

This comment has been minimized.

Copy link
Member

@wooorm wooorm commented Sep 23, 2019

Not clear to me that you need to keep up with the latest ignore, unless the previous behavior was knowingly buggy and/or deficient.

Keeping up to date is not necessarily a good effort in and off itself, but from what I know, Node/npm projects typically only have one release line, therefore using the latest semver ^ range includes bug fixes, whereas using the previous range would not.


It’s a bit late here so I’d rather work on this tomorrow, but if you have time to work on this, I’d appreciate a PR with a failing test and I can work on a fix tomorrow!

@kgryte

This comment has been minimized.

Copy link
Author

@kgryte kgryte commented Sep 23, 2019

The regular expression in ignore:

const REGEX_TEST_INVALID_PATH = /^\.*\/|^\.+$/

matches absolute file paths via

^\.*\/

and as documented in the code.

@kgryte

This comment has been minimized.

Copy link
Author

@kgryte kgryte commented Sep 23, 2019

Re: release lines. Understood and recognized.

Not clear to me how you'll want to address this atm.

@wooorm

This comment has been minimized.

Copy link
Member

@wooorm wooorm commented Sep 23, 2019

Not sure either, yet 🤷‍♂️

@wooorm

This comment has been minimized.

Copy link
Member

@wooorm wooorm commented Oct 1, 2019

Say I place your ignore file in unified-engine/path/to/.ignore, and have a markdown file at unified-engine/path/to/file.md, running:

$ pwd
/Users/tilde/oss/unified-engine
$ ./node_modules/.bin/remark --ignore-path=./path/to/.ignore ./path/to/file.md
asdasdasd
path/to/file.md: no issues found

...yields no problems. I need more information about what you are actually doing to debug this.

@kgryte

This comment has been minimized.

Copy link
Author

@kgryte kgryte commented Oct 6, 2019

I just ran the following command and encountered the following results:

DEBUG=* ./node_modules/.bin/remark --ignore-path=./etc/remark/.remarkignore ./lib/node_modules/\@stdlib/blas/base/sscal/README.md 
  unified-engine:configuration Looking for `[ '.remarkrc', '.remarkrc.js', '.remarkrc.yml', '.remarkrc.yaml' ]` configuration files +0ms
  unified-engine:configuration Looking for `remarkConfig` fields in `package.json` files +6ms
  unified-engine:find-up Checking given file `/path/to/etc/remark/.remarkignore` +0ms
  unified-engine:find-up Read given file `/path/to/etc/remark/.remarkignore` +5ms
RangeError: path should be a `path.relative()`d string, but got "../../lib/node_modules/@stdlib/blas/base/sscal/README.md"

If I run the command without specifying the ignore-path, I do not encounter any errors.

@wooorm

This comment has been minimized.

Copy link
Member

@wooorm wooorm commented Oct 6, 2019

This looks pretty similar to my case above, but for me it works, so I need more information to reproduce your issue: can I get the code project? What versions of node / deps? Etc

@kgryte

This comment has been minimized.

Copy link
Author

@kgryte kgryte commented Oct 6, 2019

Maybe the issue is that the ignore file is not in the current working directory, but a child directory of a sibling folder. The relative path walking in the error message matches ./etc/remark depth. So it looks like it may be resolving the README relative to the ignore file.

@kgryte

This comment has been minimized.

Copy link
Author

@kgryte kgryte commented Oct 6, 2019

See the node version and deps version in the OP.

The project is stdlib. Here is the full command I run locally which generates the error:

DEBUG=* ./node_modules/.bin/remark --ignore-path=./etc/remark/.remarkignore ./lib/node_modules/\@stdlib/blas/base/sscal/README.md

from the top-level project directory.

@kgryte

This comment has been minimized.

Copy link
Author

@kgryte kgryte commented Oct 6, 2019

I can confirm that, if I place the .remarkignore file in the current working directory, instead of the sibling path, then I do not encounter any errors.

@wooorm

This comment has been minimized.

Copy link
Member

@wooorm wooorm commented Oct 6, 2019

Ah that'll help, thanks!

@kgryte

This comment has been minimized.

Copy link
Author

@kgryte kgryte commented Oct 6, 2019

However, the error still remains that ignore files which do not reside along the file path result in runtime errors.

@wooorm

This comment has been minimized.

Copy link
Member

@wooorm wooorm commented Oct 6, 2019

Right, we can work on that now we know the issue!

@foray1010

This comment has been minimized.

Copy link

@foray1010 foray1010 commented Oct 31, 2019

@wooorm I am working on a PR to fix this issue, but wanna clarify some concepts.

say we have this folder structure:

/
    a/
        one.txt
    b/
        .fooignore
        two.txt
    c/
        .fooignore
    .fooignore

/a/one.txt is tested by /.fooignore
/b/two.txt is tested by /b/.fooignore

in the same scenario, if specified --ignore-path=c/.fooignore, I would expect:
/a/one.txt is tested by /c/.fooignore
/b/two.txt is tested by /c/.fooignore
And most importantly, any content in /c/.fooignore will be treated as relative to current pwd

so if we wanna ignore all txt files in /c/.fooignore, it should be written like:

a/one.txt
b/two.txt

My point is

  1. custom ignore path should override ALL ignore files
  2. content in custom ignore path is relative to pwd

agree?

@wooorm

This comment has been minimized.

Copy link
Member

@wooorm wooorm commented Oct 31, 2019

Hey @foray1010, thanks for working on this!

  1. In my understanding, an explicit, given, ignore path currently already overwrites normally found ignore files?
  2. I think that given ignore files should still ignore from the place that file was found: a typically use case for different ignore files is to alias .gitignore to the ignore file, and that should still work.
@foray1010

This comment has been minimized.

Copy link

@foray1010 foray1010 commented Nov 1, 2019

I think that given ignore files should still ignore from the place that file was found

Do you mean the patterns in given ignore files should be relative to the location of themselves?

I would like to create a npm package that provide ignore files so that I don't have to sync them on all my repos
I would expect remark --ignore-path=node_modules/my-package/remarkignore could work like I provided a .remarkignore on repo's root.
This logic works for eslint, stylelint and prettier, so I hope unified could work the same way

@wooorm

This comment has been minimized.

Copy link
Member

@wooorm wooorm commented Nov 1, 2019

Hmm, I see a couple different use cases for custom ignore path:

./cli.js --ignore-path=.gitignore # To use the patterns in a file with a different name # A
./cli.js --ignore-path=config/.remarkignore # So all config is in a different place # B
./cli.js --ignore-path=node_modules/my-config/.remarkignore # C
  • For each option, typically the CWD doesn’t matter: **/*.md ignores every .md file.
  • B and C are essentially the same, except that in one use case the patterns can be from the file itself (B), whereas with the other you’d want them relative to the CWD (C)
  • One reason to keep resolving from the ignore file is that rc files (that resolve plugins) also work that way. Using the same mechanism for both makes sense
  • Ignore files are looked for upwards when not given (e.g., you can have a /Users/alpha/.remarkignore), I also think it makes sense for that to resolve from its location instead of the CWD
  • If the CWD is a subdirectory of your project (say you’re in my-project/test/, and there is a my-project/.remarkignore), I think it also makes sense to resolve from that ignore file instead of from the CWD

Are you sure ESLint ignores relative to the CWD in case of custom ignore paths? I didn’t find that in the docs.

@wooorm

This comment has been minimized.

Copy link
Member

@wooorm wooorm commented Nov 1, 2019

Oh and could you expand on what you want to put in your ignore?

@foray1010

This comment has been minimized.

Copy link

@foray1010 foray1010 commented Nov 2, 2019

here is an example of node_modules/my-package/remarkignore

build/
coverage/
dist/
node_modules/
out/
CHANGELOG.md
@foray1010

This comment has been minimized.

Copy link

@foray1010 foray1010 commented Nov 5, 2019

@wooorm I created a demo repo to show how eslint, prettier and stylelint work: https://github.com/foray1010/test-ignore-files

you can try by

# patterns are relative to cwd, it successfully ignored demo.js & demo.css
yarn cwd:eslint
yarn cwd:prettier
yarn cwd:stylelint

# patterns are relative to the ignore files, it failed to ignore demo.js & demo.css
yarn relative_patterns:eslint
yarn relative_patterns:prettier
yarn relative_patterns:stylelint
@wooorm

This comment has been minimized.

Copy link
Member

@wooorm wooorm commented Nov 5, 2019

🤔 yarn cwd:stylelint fails for me and yarn relative_patterns:stylelint works?

@wooorm

This comment has been minimized.

Copy link
Member

@wooorm wooorm commented Nov 5, 2019

Also: the big caveat here is that yarn and npm scripts execute from the project root, as in, the place that has the package.json file.
Say you have a subdirectory, tests/, that you are in, then yes, yarn cwd:eslint works.
It looks like ESLint has advanced logic to create a base directory from the common ancestor of CWD and the ignore file.

@wooorm

This comment has been minimized.

Copy link
Member

@wooorm wooorm commented Nov 5, 2019

Prettier doesn’t does “work” with the expected CWD (so say we are in tests/, had tests/demo.js and tests/example.js, and in node_modules/cwd/.prettierignore is demo.js):

$ ../node_modules/.bin/prettier --ignore-path=../node_modules/cwd/.prettierignore --check *.js
Checking formatting...
[error] No matching files. Patterns tried: *.js !**/node_modules/** !./node_modules/** !**/.{git,svn,hg}/** !./.{git,svn,hg}/**
All matched files use Prettier code style!

Note that demo.js is ignored. I do believe this behaviour is wrong though (and remark, ESLint, stylelint seem to agree?)

kgryte added a commit to stdlib-js/stdlib that referenced this issue Nov 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.