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

Prefer relative import instead of alias if the import is a subpath #160

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dword-design
Copy link

@dword-design dword-design commented Apr 14, 2021

Summary

There is an inconsistent case when dealing with relative subpath imports.

When importing ./foo/bar, the import is kept and not replaced, which is good. When importing alias/bar however, the import is not replaced back to ./foo/bar, leading to non-deterministic behavior because the convention depends on the current code state. It should be replaced back to ./foo/bar. Because then always holds:

  • If the imported file is in a subpath of the importing file, use a relative import
  • If the imported file is not in a subpath of the importing file, use the alias import

This has a few implications though. The current implementation is kinda 'specialized' on a single fixer, which is why the fixer is decoupled from the error detection. This will lead to a lot of duplicated code because the error detection has to be replicated in the fixer.

Actually I found this plugin recently and implemented my own one before, so I thought the best idea is to put both together. This PR includes some refactoring and the fixes I already had in my project.

  • Instantiate the fixer right in the traversal function
  • Use the actual babel-plugin-module-resolver logic to resolve the aliases
  • Make it windows-compatible. The current state will generate \ separators in the fixed import paths

Would be great to have this merged! If there are questions or open things to be done, feel free to ask.

@dword-design
Copy link
Author

@HeroProtagonist Let me know if there is anything left to be done.

@HeroProtagonist
Copy link
Owner

Hi @dword-design,

Thanks for the information and PR. I like the idea of using the actually library to resolve aliases.

The rule is written to not do anything with imports at the same level (./)
The functionality you describe seems like it would be good to add as an option, rather than default behavior.

#161 has a windows fix that will get merged in

@dword-design
Copy link
Author

dword-design commented May 22, 2021

Hey @HeroProtagonist, the PR should also keep imports at the same level at ./. If it's not like this, I can adjust it again. The PR should keep the behavior like it is and only make alias imports relative in case they are on the same level or in a subpath. So it's supposed to be a bugfix originally :P.

Regarding the windows, alright good to see there is another PR for it. I'd say wait for it until we have a state here that we can merge and then we can see how to deal with the windows paths (merge from default branch or do something else).

Thanks for replying, let me know what you think.

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.

2 participants