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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[New]: add no-relative-packages #1860

Merged
merged 1 commit into from Jan 31, 2021
Merged

[New]: add no-relative-packages #1860

merged 1 commit into from Jan 31, 2021

Conversation

tapayne88
Copy link
Contributor

Duplicating #966 and hopefully fixed it 馃 (sorry if this isn't the process).

@panrafal @tapayne88 @ljharb
[New]: add no-relative-packages
6f5c52c 
Use this rule to prevent importing packages through relative paths.

It's useful in Yarn/Lerna workspaces, were it's possible to import a sibling package using `../package` relative path, while direct `package` is the correct one.

Co-authored-by: Rafal Lindemann <rl@stamina.pl>
Co-authored-by: Tom Payne <tom@tompayne.dev>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
@coveralls
Copy link

coveralls commented Jul 23, 2020 •
edited

Coverage Status

Coverage increased (+5.3%) to 75.918% when pulling 6f5c52c on tapayne88:feature/no-relative-packages into 319d0ca on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 97.837% when pulling 0aec1d3 on tapayne88:feature/no-relative-packages into 3e65a70 on benmosher:master.

@tapayne88 tapayne88 force-pushed the feature/no-relative-packages branch 6 times, most recently from 3467f06 to 300a5b3 Compare July 23, 2020 22:14
@tapayne88 tapayne88 mentioned this pull request Jul 24, 2020
Merged
@tapayne88
Copy link
Contributor Author

@ljharb is there anything I can do to get this over the line?

@wycats
Copy link

wycats commented Dec 3, 2020 •
edited

@tapayne88 I tried this out from your branch and it improved my workflow dramatically.

One recommendation: right now, the warning suggests importing from package-name/index instead of package-name. It seems like that ought not happen.

One wrinkle: in my case, I am not importing from the same location as the main entry point (which is dist/index.js) because the mistaken import is accidentally the index.ts in the root of the package. I have types pointing at index.ts, but that is probably not a perfect heuristic. In a situation where the import is pointing at an index.ts file that isn't the main entry point, perhaps it's just fine to suggest package-name?

@tapayne88
Copy link
Contributor Author

hey @wycats, thanks for the feedback! To be honest, the bulk of the work was originally done by @panrafal but I'll try set aside some time to look into your recommendation - it does sound closer to what we want.

If I'm understanding your wrinkle correctly, I think node module resolution should use the "main" field in the package.json first (if available) and resort to the index.js/index.ts second before heading up the filesystem in search of more node_modules folders so I think what you're seeing is the desired behaviour.

@wycats
Copy link

wycats commented Dec 9, 2020

@tapayne88 If it finds index.ts in the absence of a types field (even in the presence of main field), I think that would be ok.

@wycats
Copy link

wycats commented Dec 9, 2020

@ljharb what do you think of this PR?

ljharb
ljharb approved these changes Jan 31, 2021
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.

Looks great! Went ahead and rebased, and refactored to use the new moduleVisitor pattern.

@ljharb ljharb force-pushed the feature/no-relative-packages branch from e6533d2 to 6f5c52c Compare January 31, 2021 17:05
@ljharb ljharb merged commit 6f5c52c into import-js:master Jan 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

Assignees
No one assigned
Labels
accepted rule proposal semver-minor
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tapayne88 @coveralls @wycats @ljharb @panrafal