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

[New] no-unresolved: add caseSensitiveStrict option #1262

Merged
merged 2 commits into from
Sep 13, 2021

Conversation

sergei-startsev
Copy link
Contributor

Fix #1259 issue. The code above should be identified as invalid by import/no-unresolved rule with enabled caseSensitiveStrict option:

/*eslint import/no-unresolved: [2, { caseSensitiveStrict: true }]*/

// Absolute paths
import Foo from `/Users/fOo/bar/file.js` // reported, /Users/foo/bar/file.js
import Foo from `d:/fOo/bar/file.js` // reported, d:/foo/bar/file.js

// Relative paths, cwd is Users/foo/
import Foo from `./../fOo/bar/file.js` // reported

@coveralls
Copy link

coveralls commented Jan 18, 2019

Coverage Status

Coverage increased (+0.004%) to 97.311% when pulling acb20e5 on sergei-startsev:no-unresolved-1259 into 767f01a on benmosher:master.

@sergei-startsev
Copy link
Contributor Author

@benmosher @ljharb Have a chance to take a look? The changes are behind the option, no breaking changes are expected.

@benmosher
Copy link
Member

Ah interesting, if just plain caseSensitive is not properly checking paths with relative bits, I think that is a bug and a new option is not necessary. @ljharb can check my math on that though.

But my recommendation would be to make caseSensitive report on your case as well.

@benmosher
Copy link
Member

Ohhh wait sorry I remember now. This cwd behavior was added because people’s case-insensitive terminal was throwing for all resolves. Hmm...

@benmosher
Copy link
Member

Feels like any explicit path segment should be checked, regardless of the CWD. Not sure how exactly that would work though.

@sergei-startsev
Copy link
Contributor Author

@benmosher right, that's how caseSensitive worked initially before ignoring cwd part.

@ljharb
Copy link
Member

ljharb commented Aug 8, 2021

@sergei-startsev sorry for the long delay here. if you'd mind rebasing, i'll be happy to take a fresh look.

@ljharb ljharb marked this pull request as draft August 8, 2021 05:57
@sergei-startsev sergei-startsev marked this pull request as ready for review August 23, 2021 19:38
@sergei-startsev
Copy link
Contributor Author

@ljharb The PR has been rebased, take a look.

@ljharb
Copy link
Member

ljharb commented Aug 23, 2021

@sergei-startsev what happens if someone sets caseSensitive to false, but caseSensitiveStrict to true? Same question if both are true?

Maybe it would make more sense if the option was something that doesn't imply it's a superset of caseSensitive?

@ljharb ljharb added package: utils eslint-module-utils package semver-minor labels Aug 23, 2021
ljharb pushed a commit to sergei-startsev/eslint-plugin-import that referenced this pull request Aug 23, 2021
@ljharb ljharb changed the title Fix #1259 for no-unresolved, add caseSensitiveStrict option [New] no-unresolved: add caseSensitiveStrict option Aug 23, 2021
@sergei-startsev
Copy link
Contributor Author

sergei-startsev commented Aug 23, 2021

@ljharb Good point, rethought caseSensitiveStrict, it'll win in both cases now (added one more test to cover this case).

ljharb pushed a commit to sergei-startsev/eslint-plugin-import that referenced this pull request Aug 23, 2021
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks!

@sergei-startsev
Copy link
Contributor Author

@ljharb Do you plan to merge the PR yourself?

@ljharb
Copy link
Member

ljharb commented Aug 24, 2021

@sergei-startsev yep! i'm holding off because i want to find a fix for #2199, so i can release a v2.24 patch release, before merging this in for a v2.25.

ljharb pushed a commit to sergei-startsev/eslint-plugin-import that referenced this pull request Aug 25, 2021
@ljharb
Copy link
Member

ljharb commented Aug 26, 2021

EOL status of a platform is irrelevant (node 10 is EOL also).

However, the failure might be in eslint < 5, and the node version might be irrelevant.

@sergei-startsev
Copy link
Contributor Author

It's hard to say why they failed, I can't debug this on macOS.

@ljharb
Copy link
Member

ljharb commented Aug 26, 2021

On my Mac, on node 16 but eslint 4, I can reproduce it - I'm happy to throw in any console logs that would help you and provide the output :-)

@sergei-startsev
Copy link
Contributor Author

sergei-startsev commented Aug 26, 2021

Could we just skip these tests if eslint <5?

@ljharb
Copy link
Member

ljharb commented Aug 26, 2021

That would be a reasonable fallback, but usually I prefer to do that when the reason they fail is "eslint < 5 can't support that syntax" rather than "we don't know why they're failing".

@sergei-startsev
Copy link
Contributor Author

OK, I'll try to investigate it more. In the meantime I'd appreciate it if you could provide any details about fileExistsWithCaseSync execution for detecting case should include parent folder path test (tests/src/core/resolve.js:343).

@ljharb
Copy link
Member

ljharb commented Aug 26, 2021

In that test case, f is /UPPER/CASE/PATH/TO/REPO/tests/files/jsx/MyUnCoolComponent.jsx. The UPPERCASED part does not match the casing, but since Macs are case insensitive, the path is treated identically to the way the properly-cased one would be.

In the same test case, inside fileExistsWithCaseSync, filenames is:

[
  'AnotherComponent.jsx',
  'App.js',
  'FooES7.js',
  'MyCoolComponent.jsx',
  'MyUnCoolComponent.jsx',
  'named.jsx'
]

and parsedPath is:

{
  root: '/',
  dir: '/UPPER/CASE/PATH/TO/REPO//tests/files/jsx',
  base: 'MyUnCoolComponent.jsx',
  ext: '.jsx',
  name: 'MyUnCoolComponent'
}

and it repeats as it navigates upwards until it gets to:

{
  root: '/',
  dir: '/UPPER/CASE/PATH/TO/REPO',
  base: 'tests',
  ext: '',
  name: 'tests'
}

@sergei-startsev
Copy link
Contributor Author

Well, I don't see how ESLint version can affect path resolving here...

@ljharb
Copy link
Member

ljharb commented Aug 26, 2021

Neither do I - but actually, when I run on eslint 7 i get the same console results and the test fails too - so now I'm confused how they passed on travis at all.

@ljharb
Copy link
Member

ljharb commented Aug 26, 2021

I'm starting to wonder if it's just not possible for a "strict" option to work on a case-insensitive file system (where it's most valuable), if there's no possible way to get a canonical capitalization?

@sergei-startsev
Copy link
Contributor Author

Is there an option to add a windows runner to the matrix to check it?

@ljharb
Copy link
Member

ljharb commented Aug 26, 2021

I'm not sure how to do it in travis, but it should theoretically be possible to do it in github actions. however, i haven't set up windows support on ljharb/actions/node/install yet.

@sergei-startsev
Copy link
Contributor Author

sergei-startsev commented Aug 26, 2021

Well, then I don't know how to prove that it actually works 😞 I'd be glad to hear any ideas.

Here's what I have on windows (tests are always green):

parsedPath {
  root: 'D:\\',
  dir: 'D:\\REPOS',
  base: 'ESLINT-PLUGIN-IMPORT',
  ext: '',
  name: 'ESLINT-PLUGIN-IMPORT'
}
filenames: [
  'eslint-plugin-import',
  ...
]

sergei-startsev added a commit to sergei-startsev/eslint-plugin-import that referenced this pull request Aug 27, 2021
@sergei-startsev
Copy link
Contributor Author

@ljharb Travis hasn't run again – Owner import-js does not have enough credits. Could you fix it, so I can check some logs?

@ljharb
Copy link
Member

ljharb commented Aug 27, 2021

woof, those things run out quick. i'll ping travis and ask for more.

ljharb pushed a commit to sergei-startsev/eslint-plugin-import that referenced this pull request Sep 2, 2021
@ljharb
Copy link
Member

ljharb commented Sep 2, 2021

@sergei-startsev travis should be working again; i've rebased this branch so you can see the logs.

@codecov
Copy link

codecov bot commented Sep 3, 2021

Codecov Report

Merging #1262 (35bd977) into main (7784948) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1262      +/-   ##
==========================================
+ Coverage   84.27%   84.28%   +0.01%     
==========================================
  Files          93       93              
  Lines        2969     2971       +2     
  Branches      879      881       +2     
==========================================
+ Hits         2502     2504       +2     
  Misses        467      467              
Impacted Files Coverage Δ
src/rules/no-unresolved.js 100.00% <100.00%> (ø)
utils/resolve.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7784948...35bd977. Read the comment docs.

@sergei-startsev
Copy link
Contributor Author

sergei-startsev commented Sep 3, 2021

@ljharb Not sure what went wrong last time, but the checks are green on all platforms now (thanks for fixing travis and adding appveyor). The commit with logs was reverted (not sure why codecov hasn't rerun).

edited: codecov report was finally updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: utils eslint-module-utils package semver-minor
Development

Successfully merging this pull request may close these issues.

Incorrect file resolution for paths higher than CWD
4 participants