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

fix: only apply to modules #3

Merged
merged 3 commits into from
Jan 31, 2023
Merged

Conversation

tpluscode
Copy link
Contributor

This is an attempt to fix #1

I would like to add some tests but this is my first time writing code for an eslint plugins and not sure how to proceed

About the change itself, it find the actual module in the filesystem and applies to rule only when a module exists with one of the extensions

For example, given code like

import foo from './bar.json'

An error will be shown when there is a source file bar.json.js or bar.json.ts but not when it's simply bar.json

I'm not sure if it's correct to also include jsx and tsx in the extensions to check.

@tpluscode
Copy link
Contributor Author

Ping @jordansexton ?

@jordaaash
Copy link
Collaborator

@tpluscode Ah, thanks for the ping, didn't see this. Will review today.

@tpluscode
Copy link
Contributor Author

Super, thanks

Copy link
Collaborator

@jordaaash jordaaash left a comment

Choose a reason for hiding this comment

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

Hmm, I don't think we need to actually check for the existence of the file, do we? Importing a file that doesn't exist should be allowed, as long as the required extension (.js) is provided. This should typically get handled by a different eslint rule, which will have a more helpful error description.

I don't know if this is a good idea or not, but a possible fix could be to look for an extension at the end of the filename, and if one is there, look for a corresponding file at that full path. If the file exists, don't enforce the .js extension, but just allow any extension to be imported, and assume the dev knows what they are doing.

@tpluscode
Copy link
Contributor Author

Aha, I wrote that lengthy reply before realising I misunderstood your point. 😅

Yes, you're right, a file which does no exist should have the .js suffix added which will inevitably lead to a different rule kicking in, or TS compiler error, ot runtime loader error. Not this plugin's concern

There are quite some cases, some of which are not handled by this PR not the base branch. It would really be good to add unit tests but I do not have experience testing eslint rules. Do you?

@tpluscode
Copy link
Contributor Author

What do you think about my updated code @jordansexton ?

Copy link
Collaborator

@jordaaash jordaaash left a comment

Choose a reason for hiding this comment

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

I reviewed again, but it looks like the first review wasn't fully addressed.

Looking @ #1, it seems that the problem is that imports of files with explicit extensions (.json, maybe others?) should not have .js extensions added.

Is this right? If so, it seems like the fix could be as simple as checking for a file extension on the import. If it's there, and perhaps if it's not .js/x or .ts/x, we don't do anything with that import. I don't think we care about the actual file, right?

index.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@jordaaash jordaaash merged commit ff6bfb1 into solana-labs:master Jan 31, 2023
@jordaaash
Copy link
Collaborator

Published in v0.1.2! Please let me know if it works well for you, and thank you for working on this!

@tpluscode tpluscode deleted the only-module branch February 1, 2023 08:13
@tpluscode
Copy link
Contributor Author

Super, thanks for releasing

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.

False-negative on JSON import
2 participants