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

Adds support for babel-module-resolver #5

Merged

Conversation

leorossi
Copy link
Contributor

This is a first attempt to solve #3

I used babel-plugin-module-resolver and find-babel-config to trigger babel plugin to resolve a file which is imported.

I tested with a sample project but I am guessing there is still a little work to do.

I am checking in fileResolver:42 whether the file might be a local dependency, but actually this will be triggered even with a import { something} from 'an-npm-package'

Maybe this check should be done after we are sure it's not a node module?

}

function checkBabelModuleResolver(filePath, currentFile) {
const loadedConfig = findBabelConfig.sync(currentFile);
if (loadedConfig) {
Copy link
Owner

Choose a reason for hiding this comment

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

maybe also add some conditions if the config file does not contain plugins for example:
loadedConfig && loadedConfig.config.plugins

@@ -18,6 +39,10 @@ const resolveSync = enhancedResolve.create.sync({
*/
function resolve(relPath = "", currentFile = "") {
const currentDir = path.dirname(currentFile);
if (!relPath.match(/^(\.|\/)/)) {
Copy link
Owner

Choose a reason for hiding this comment

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

The responsibility of the method checkNodeModules is exactly the same as this condition, however checkNodeModules also checks for windows path too (for example, c:/),
I would suggest, instead of this condition use checkNodeModules. However the name checkNodeModules is not meaningful anymore, maybe rename it isAbsolutePath

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to be clear, are there any other options than these?

1) import foo from 'bar'
2) import foo from '../../../bar'
3) import foo from '/usr/local/npm/bar' # or 'c:/Users/foobar/npm/bar'
  1. refers to a node module OR a babel-resolved model
  2. is a relative path
  3. is an absolute path

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, I guess that it covers it all:)
Just one point the first case can also be webpack alias and package.json aliases too:)

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 found another case which is import foo from 'includes/bar which can be resolved by babel-module-resolver, so I had to change a bit this piece of code

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 found another case which is import foo from 'includes/bar which can be resolved by babel-module-resolver, so I had to change a bit this piece of code

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 found another case which is import foo from 'includes/bar which can be resolved by babel-module-resolver, so I had to change a bit this piece of code

@@ -18,6 +39,10 @@ const resolveSync = enhancedResolve.create.sync({
*/
function resolve(relPath = "", currentFile = "") {
const currentDir = path.dirname(currentFile);
if (!relPath.match(/^(\.|\/)/)) {
Copy link
Owner

Choose a reason for hiding this comment

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

Also about this if, I suggest moving it after line 53 inside catch phrase, in that way all the normal cases (1. relative imports, 2. node_modules imports) are handled first, then if it is not found there it can look for babel aliases

@M-Izadmehr
Copy link
Owner

@leorossi Thank you for the amazing pull request, I think it is a really useful feature.
I added a few comments.

@leorossi leorossi force-pushed the feature/support-babel-module-resolver branch from b6497e5 to 7e083a3 Compare February 19, 2020 17:06
@leorossi
Copy link
Contributor Author

@M-Izadmehr I added a test/main.js file that is not really a test, it's a quicker way to verify that the example project in test/fixtures is correctly handled by this repo.

@leorossi leorossi mentioned this pull request Feb 19, 2020
@leorossi leorossi force-pushed the feature/support-babel-module-resolver branch from 7e083a3 to 6e7b3b3 Compare February 19, 2020 17:19
@mcollina
Copy link

@M-Izadmehr are there any updates on this one?

@M-Izadmehr
Copy link
Owner

Thanks @leorossi for the help.
I will merge this PR, thanks for the contribution.

@M-Izadmehr M-Izadmehr merged commit 46e51dd into M-Izadmehr:master Jun 27, 2020
@M-Izadmehr
Copy link
Owner

🎉 This PR is included in version 1.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants