Use getPhysicalFilename() instead of getFilename() if available (ESLint 7.28+)#2160
Merged
Merged
Conversation
pmcelhaney
commented
Jul 21, 2021
Comment on lines
+52
to
+64
| it('falls back to getFilename() when getPhysicalFileName() is not available (ESLint < 7.28)', function () { | ||
| const testContext = utils.testContext({ 'import/resolver': './foo-bar-resolver-v1' }); | ||
|
|
||
| expect(resolve( '../files/foo' | ||
| , Object.assign({}, testContext, { getFilename: function () { return utils.getFilename('foo.js'); } }), | ||
| )).to.equal(utils.testFilePath('./bar.jsx')); | ||
|
|
||
| expect(resolve( '../files/foo' | ||
| , Object.assign({}, testContext, { getFilename: function () { throw new Error('Should call getPhysicalFilename() instead of getFilename()'); }, getPhysicalFilename: function () { return utils.getFilename('foo.js'); } }), | ||
| )).to.equal(utils.testFilePath('./bar.jsx')); | ||
|
|
||
|
|
||
| }); |
Contributor
Author
There was a problem hiding this comment.
Added this test to make sure the fallback behavior is working. Otherwise the tests have been updated to use getPhysicalFilename() instead of getFilename().
ljharb
requested changes
Jul 22, 2021
ljharb
left a comment
ljharb
left a comment
Member
There was a problem hiding this comment.
Thanks!
please swap all the a && a() || b() for a ? a() : b(); plus the tests comment
|
|
||
| export function getContextPackagePath(context) { | ||
| return getFilePackagePath(context.getFilename()); | ||
| return getFilePackagePath((context.getPhysicalFilename && context.getPhysicalFilename()) || context.getFilename()); |
Member
There was a problem hiding this comment.
Suggested change
| return getFilePackagePath((context.getPhysicalFilename && context.getPhysicalFilename()) || context.getFilename()); | |
| return getFilePackagePath(context.getPhysicalFilename ? context.getPhysicalFilename() : context.getFilename()); |
| if (deepLookup.path.length > 1) { | ||
| const deepPath = deepLookup.path | ||
| .map(i => path.relative(path.dirname(context.getFilename()), i.path)) | ||
| .map(i => path.relative(path.dirname((context.getPhysicalFilename && context.getPhysicalFilename()) || context.getFilename()), i.path)) |
Member
There was a problem hiding this comment.
Suggested change
| .map(i => path.relative(path.dirname((context.getPhysicalFilename && context.getPhysicalFilename()) || context.getFilename()), i.path)) | |
| .map(i => path.relative(path.dirname(context.getPhysicalFilename ? context.getPhysicalFilename() : context.getFilename()), i.path)) |
Contributor
Author
There was a problem hiding this comment.
Yes, that's better. Let me know how you want to proceed on the tests and I'll make both changes at the same time. Thanks for the feedback!
|
|
||
| expect(resolve( '../files/foo' | ||
| , Object.assign({}, testContext, { getFilename: function () { return utils.getFilename('foo.js'); } }), | ||
| , Object.assign({}, testContext, { getPhysicalFilename: function () { return utils.getFilename('foo.js'); } }), |
Member
There was a problem hiding this comment.
rather than modifying the existing tests, please leave these alone, so that the new getPhysicalFilename duplicated tests are clearly additions
Contributor
Author
There was a problem hiding this comment.
Okay, which do you prefer?
- Duplicate all of the existing tests so that the only difference is
getFilename()vs.getPhysicalFilename() - Revert the changes so that we're testing all of the use cases for
getFilename()and not exercisinggetPhysicalFilename()except for the couple of tests demonstrating thatgetPhysicalFilename()is the preferred API andgetFIlename()is a fallback. - Same as 1, but add
getFilename() { throw new Error(...) }to the mock context on all the new tests rather than have a separate test for the fallback.
Member
There was a problem hiding this comment.
I was originally thinking option 1, but i really do like option 3.
ljharb
force-pushed
the
physical-filename
branch
from
August 3, 2021 03:43
8c3aafa to
2f0243f
Compare
ljharb
approved these changes
Aug 3, 2021
ljharb
force-pushed
the
physical-filename
branch
2 times, most recently
from
August 3, 2021 15:40
a4c0ab4 to
bfab4cc
Compare
Member
|
(Had to update the tests so the getPhysicalFilename stuff only applies in eslint v7.28+) |
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Starting with 7.28.0, ESLint has a new feature distinguishing the physical filename from the virtual filename.
When there's a difference, this plugin needs the real, physical filename. (See
eslint/eslint#11989)
I ran into the problem when I noticed
no-unresolvedwas yielding false positives inside of code blocks in Storybook / Markdown files and helped push the necessary change through in ESLint.To maintain backwards compatibility, this change uses
context.getPhysicalFilename()if available and otherwise falls back togetFilename().