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

feat(import-target): Add resolution error reason #231

Closed

Conversation

scagood
Copy link

@scagood scagood commented Apr 8, 2024

closes #182

@scagood
Copy link
Author

scagood commented Apr 9, 2024

I was also thinking something along the lines of:

exports.messages = {
    notFound: '{{resolveError}}',
}

@aladdin-add
Copy link

SGTM! 👍

@scagood
Copy link
Author

scagood commented Apr 15, 2024

mmm, these failures are interesting, as on a mac the file did resolve as that os is case insensitive 🤔

image

This error is getting triggered:

// This should never happen... this is just a fallback for typescript
target.resolveError ??= `"${target.name}" is not found`

@aladdin-add
Copy link

can we just assert the messageId, not message?

@aladdin-add
Copy link

we can just assert the messageId, not the full message. wdyt?

Comment on lines +46 to +48
data: {
resolveError: `Can't resolve '${name}' in '${fixture(dir)}'`,
},

Choose a reason for hiding this comment

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

Suggested change
data: {
resolveError: `Can't resolve '${name}' in '${fixture(dir)}'`,
},

@scagood
Copy link
Author

scagood commented Apr 16, 2024

The issue is that ./A.js will resolve sometimes, OS dependent 👀

@aladdin-add
Copy link

image

But when I look at the execution results, it is returning different error messages under some os. So I suggest to only assert the messageId. am I missing something?

@scagood
Copy link
Author

scagood commented Apr 17, 2024

The issue is that on your machine, ./A does actually resolve in require

On my linux machine (which is case sensitive) it does not, so the error is correct:
image

@scagood
Copy link
Author

scagood commented Apr 17, 2024

I have just stripped out everything except for the enhanced-resolve checks (f9e454d)

(Clicking the image should take you to the tests from which the screen shot is taken)

On linux (that is case sensitive), everything is fine:
image

On mac (that is case insensitive), the ./A.js is resolvable:
image


This means that on linux, its correct that .tests/fixtures/no-missing/A.js does not get loaded:

image

And its correct on mac that .tests/fixtures/no-missing/A.js does get loaded:

image

@aladdin-add
Copy link

aladdin-add commented Apr 17, 2024

we can ask the enhanced-resolve to support a case-sensitive option.

Up until that point, it seems that we can only ignore these test cases under certain os. The rule tester does support only property, but not skip, we will need to implement it in our side. wdyt?

https://eslint.org/docs/latest/integrate/nodejs-api#ruletester

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: import-target mutes resolution errors
2 participants