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

[Fix] extensions: handle . and .. properly #2778

Merged
merged 1 commit into from Jul 26, 2023
Merged

[Fix] extensions: handle . and .. properly #2778

merged 1 commit into from Jul 26, 2023

Conversation

benasher44
Copy link
Contributor

The issue was isExternalRootModule was returning true for these, since they had no / in them. This fixes that.

Closes #2777

@benasher44 benasher44 changed the title Fix extensions import for . or .. Fix extensions rule for . or .. May 8, 2023
@benasher44 benasher44 changed the title Fix extensions rule for . or .. Fix extensions rule for . and .. May 8, 2023
ljharb
ljharb reviewed May 8, 2023
View reviewed changes
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.

Can we also add some test cases with the "always" option where . and .. refer to dirs with package.json, and show that these don't error?

@benasher44
Copy link
Contributor Author

It seems like that may be an existing bug. I'm open to fixing it, but it looks like the case where you import a relative package (i.e. relative path to a directory containing a package.json) is broken. Some options:

  1. Make some kind of change to typeTest. This could have wide-ranging implications and is the change I'm am probably least sure of.
  2. Add a simple check that gets the absolute path and tests whether the path is a directory containing a package.json.

Do you have any thoughts here? Additionally, I think always in this case should require you to add an extension, since it's always, whereas ignorePackages would not error.

@ljharb
Copy link
Member

ljharb commented May 8, 2023

Ideally I'd like this PR to either add tests documenting the current behavior (with a TODO comment if it's broken), or, tests matching the desired behavior and fixes for it.

While ignorePackages was intended to cover things in node_modules, i think it actually makes sense to cover a relative dir with a package.json as well. I'm wondering how large a change that would be.

@ljharb ljharb marked this pull request as draft May 8, 2023 16:06
@benasher44 benasher44 requested a review from ljharb June 25, 2023 19:22
@benasher44 benasher44 marked this pull request as ready for review June 25, 2023 19:22
@benasher44
Copy link
Contributor Author

benasher44 commented Jun 25, 2023 •
edited

I added tests that show the current behavior (says extensions are required when they shouldn't be) + TODOs

benasher44
benasher44 commented Jun 25, 2023
View reviewed changes
tests/src/rules/extensions.js
'import * as test from ".."',
].join('\n'),
filename: testFilePath('./internal-modules/plugins/plugin.js'),
options: [ 'ignorePackages' ],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about whether these should be always. I think with ignorePackages it should definitely not error, so it makes for a less ambiguous test case.

In the always + relative case it's a bit more ambiguous to me, since if we assume the relative path is in your project, you might have this set to always because you always want someone to choose a file (within the project).

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what to do here, tbh.

Copy link
Member

Choose a reason for hiding this comment

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

I'm going to merge this, and please file followup issues and/or PRs :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed #2844!

@benasher44

This comment was marked as resolved.

Sign in to view
@ljharb ljharb changed the title Fix extensions rule for . and .. [Fix] extensions: handle . and .. properly Jul 26, 2023
@ljharb ljharb merged commit d8002be into import-js:main Jul 26, 2023
162 of 163 checks passed
@benasher44 benasher44 mentioned this pull request Jul 31, 2023
Open
@benasher44
Copy link
Contributor Author

Thank you!

@benasher44 benasher44 deleted the basher/fix-dot branch July 31, 2023 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb Awaiting requested review from ljharb

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[import/extensions] Doesn't error on . or ..
2 participants
@benasher44 @ljharb