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.

Sign up for GitHub

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

Jump to bottom

standard v14 not working with files in a different directory #1384

Closed
revington opened this issue Aug 21, 2019 · 17 comments · Fixed by standard/standard-engine#228
Closed

standard v14 not working with files in a different directory #1384

revington opened this issue Aug 21, 2019 · 17 comments · Fixed by standard/standard-engine#228
Labels
accepted bug
Milestone
standard 16

Comments

@revington
Copy link

What version of this package are you using?
14.0.0
What operating system, Node.js, and npm version?
Void Linux, node 10.16.3, npm 6.9.0
What happened?
$ standard --fix $file-not-same-directory fails with

/home/pedro/.nvm/versions/node/v10.16.3/lib/node_modules/standard/node_modules/deglob/node_modules/ignore/index.js:337
  throw new Ctor(message)
  ^

RangeError: path should be a `path.relative()`d string, but got "../../tmp/crap.js"
    at throwError (/home/pedro/.nvm/versions/node/v10.16.3/lib/node_modules/standard/node_modules/deglob/node_modules/ignore/index.js:337:9)
    at checkPath (/home/pedro/.nvm/versions/node/v10.16.3/lib/node_modules/standard/node_modules/deglob/node_modules/ignore/index.js:356:12)
    at Ignore._test (/home/pedro/.nvm/versions/node/v10.16.3/lib/node_modules/standard/node_modules/deglob/node_modules/ignore/index.js:473:5)
    at Ignore.ignores (/home/pedro/.nvm/versions/node/v10.16.3/lib/node_modules/standard/node_modules/deglob/node_modules/ignore/index.js:512:17)
    at path (/home/pedro/.nvm/versions/node/v10.16.3/lib/node_modules/standard/node_modules/deglob/node_modules/ignore/index.js:516:26)
    at Array.filter (<anonymous>)
    at Ignore.filter (/home/pedro/.nvm/versions/node/v10.16.3/lib/node_modules/standard/node_modules/deglob/node_modules/ignore/index.js:520:29)
    at /home/pedro/.nvm/versions/node/v10.16.3/lib/node_modules/standard/node_modules/deglob/index.js:46:31
    at end (/home/pedro/.nvm/versions/node/v10.16.3/lib/node_modules/standard/node_modules/run-parallel/index.js:18:15)
    at done (/home/pedro/.nvm/versions/node/v10.16.3/lib/node_modules/standard/node_modules/run-parallel/index.js:22:10)

but $ standard --fix file.js works as expected
@revington
Copy link
Author

I have the same error with standard@13 but not with standard@12

@theimpostor
Copy link

theimpostor commented Sep 3, 2019 •
edited

I am seeing this error as well on macos with node@10 via homebrew:

~ node --version
v10.16.3
~ npm --version
6.11.2
~ standard --version
14.1.0
~ sw_vers
ProductName:	Mac OS X
ProductVersion:	10.13.6
BuildVersion:	17G8030

It's causing problems when standard is used with ALE because ALEFix calls standard via a temp file.

@theimpostor theimpostor mentioned this issue Sep 3, 2019
Closed
@feross
Copy link
Member

feross commented Sep 11, 2019

Can you run npm ls ignore and report the output?

@theimpostor
Copy link 

~ npm ls -g ignore
/usr/local/lib
└─┬ standard@14.1.0
  ├─┬ eslint@6.2.2
  │ └── ignore@4.0.6 
  ├─┬ eslint-plugin-node@9.1.0
  │ └── ignore@5.1.4 
  └─┬ standard-engine@12.0.0
    └─┬ deglob@4.0.1
      └── ignore@5.1.4

@revington
Copy link
Author

revington commented Sep 11, 2019 via email

@revington
Copy link
Author

revington commented Sep 11, 2019 via email

@feross
Copy link
Member

feross commented Sep 11, 2019

I think I figured out the issue. In ignore@5 there was a breaking change which we didn't handle correctly: https://github.com/kaelzhang/node-ignore#upgrade-4x---5x

@theimpostor @revington Do you mind sharing if you're passing an --ignore flag to standard, or if you specified any config options in the "standard" field in package.json. I assume that you must have done so.

@theimpostor
Copy link

theimpostor commented Sep 11, 2019 via email

@feross
Copy link
Member

feross commented Sep 11, 2019

Thanks. Just a few more questions:

  • Do you have a "standard" option in your package.json file? If yes, can you paste it here?
  • Do you have a .gitignore file in your project? If yes, can you paste it here?

@theimpostor
Copy link

No "standard" option in package.json.
Here is the .gitignore file:

.env
node_modules
firebase-debug.log

You can see my project itself if it helps - no real references to 'standard' (I am using it via npm global install).

@zwx00
Copy link

zwx00 commented Nov 9, 2019

This is still broken.

@mightyiam
Copy link
Member

@feross were you able to reproduce, please?

@coreybutler
Copy link

@feross - I came across this running standard "../src/**/*.js" --verbose. It seems standard cannot accept relative paths using dot notation. Is that intentional?

For context, I have a complex build & test environment. I've abstracted the build/test aspects into their own local modules. My project structure looks like this:

root
  /src
    files.js
  /build
    node_modules
    package.json
  /test
    node_modules
    package.json  <-- This is where npm script runs standard

I'm running standard as part of an npm script within the test directory.

If Standard isn't supposed to support this, no worries (consider this a feature request). If it is supposed to support this, the issue stems from ignore/index.js#354, where the regex is marking these relative paths as invalid.

For what it's worth, I changed that line to the following (reverse the checkPath):

image

Doing that, all worked as expected, despite the quick hack and zero impact testing. Standard still respected my rather large list of ignore patterns.

@kpeters-cbsi
Copy link

I'm seeing this as well using standard 14.3.1. Happens with or without a .gitignore

@kpeters-cbsi kpeters-cbsi mentioned this issue Feb 14, 2020
Closed
@DanSchoppe
Copy link

A debugging clue...

I worked around this by manually editing node_modules/deglob/node_modules/ignore/index.js::createFilter():

  createFilter () {
    return path => true // !this.ignores(path)
  }

To be clear, this change worked for both:

  • a globally-installed binary, which was failing on ../ relative paths even without any .gitignore in the working directory or any parent of the working directory
  • a project-installed binary, where there is a .gitignore present

Not sure what's leading up to this call to filter, but it seems broken. 🤷‍♂

hsanson pushed a commit to hsanson/ale that referenced this issue Mar 12, 2020
Fix 2816 - Standard fix does not work.
c207d6a 
The standard linter --fix fails if the file being input is not relative
to the project root (standard/standard#1384).

This MR attempts to fix this by changing the command so the input file
is relative to the project root and the output is to a temporary file.

Preliminary tests with toy javascript projects seem to indicate this
works fine.
@hsanson hsanson mentioned this issue Mar 12, 2020
Merged
@feross feross added the bug label Oct 29, 2020
@feross feross added this to the standard 16 milestone Oct 29, 2020
@feross
Copy link
Member

feross commented Oct 29, 2020

I can no longer reproduce this issue, so perhaps it was fixed upstream in ignore? In either case, we're removing deglob (which was invoking ignore) in the next version of standard, v16.

So, this issue should be fixed!

@feross feross mentioned this issue Oct 29, 2020
Merged
@feross
Copy link
Member

feross commented Oct 29, 2020

This will be fixed in standard 16

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
accepted bug
Projects
The Standard Project
Archived in project
Milestone
standard 16
Development

Successfully merging a pull request may close this issue.

BREAKING: Use eslint for pattern ignoring; remove deglob standard/standard-engine
8 participants
@revington @feross @mightyiam @coreybutler @zwx00 @theimpostor @DanSchoppe @kpeters-cbsi