Add support for resolving extensions in relative paths

[amosyuen]

Currently relative paths are always ignored, so extensions can't be resolved for relative paths. This change adds support for resolving relative paths.

[codecov]

Codecov Report

Merging #259 into master will not change coverage.
The diff coverage is 100.00%.

Impacted Files	Coverage Δ
src/resolvePath.js	100.00% <100.00%> (ø)

[tleunen]

@fatfisz What do you think?
I feel like it could slow down projects compilation if we start checking all paths?
At the same time, in big projects, my guess is that most of the paths are already using the root/alias config?

[fatfisz]

I'm more worried about surprises in resolving, but we could always release this as a RC to verify the assumptions. Otherwise I think this is a good change; I wouldn't worry about the potential increase in time.

[amosyuen]

So how should we move forward with this?

[tleunen]

I'd like the feature, I think it makes sense. Resolving is not only about custom paths, it's also about specific extensions.

Although the use case seems limited since people usually write the extension when it's not one of the "default" supported extensions.

And it wouldn't work well for the .android.js or .ios.js since the plugin doesn't know which one to use in that case. It shouldn't actually put the extension in this very specific case.

[tleunen]

Do we really need to remove this condition?

I mean.. Resolving a relative path is only about potentially adding the custom extension, right?

[amosyuen]

If we want to make sure the relative path is checked first we can change the order of the resolvers to have the relativeResolver first. Which is logically equivalent to having an if statement that explicitly runs the relative resolver.

I'm not opposed to that, but I think this is more a question of where we want the if checks of whether to run the resolver. For example, rather than have the array of resolvers, we could just have multiple if statements that decide whether to run each resolver. But I do think we should be consistent. My leaning is towards having the if checks in the resolvers.

[tleunen]

Agreed. Sorry, it's been a while since I worked in this area of the code and I didn't read it properly. resolvePath is called for every "resolvers" so indeed we have to remove it.

But then, should we actually run resolvePathFromAliasConfig and resolvePathFromRootConfig on relative paths? I don't think so, right? (cc @fatfisz)
Should relative paths only run resolvePathFromRelativeConfig to potentially resolve the custom extension?

[amosyuen]

Possibly somebody might want a relative path alias "e.g. ./lib -> some/other/path/lib". So there might be a small case for running aliases for relative paths. resolving from root, probably not.

Depending on the order we put the resolvers, the others won't run. e.g. with the way I have it now, if the path is relative, it will always run resolvePathFromRelativeConfig first, and so will never reach the other two resolvers.

[wardpeet]

i agree with @amosyuen . it might slow down resolution but perhaps we should add an exclude/include option so we can exclude/include specific directories/regexes, ....

for the android, ios case you probably have different babel configs, one that looks for ios.js & .js and another one that does for android.js and .js

[wardpeet]

And it wouldn't work well for the .android.js or .ios.js since the plugin doesn't know which one to use in that case. It shouldn't actually put the extension in this very specific case.
I believe it does, most people will have more than one config, one for android and one for ios. They will have set different extensions for each build platform.

I have something like this in config

['module-resolver', {
      extensions: [BUILD_PLATFORM + '.js', '.js']
      stripExtensions: [],
    }],

where BUILD_PLATFORM can be web, android or ios.

this PR does the trick for me already published to our private NPM

[tleunen]

I finally got a use case where I wanted to resolve a relative path as well, but with a specific alias config though. The usecase was to map all *.css files into something else that could work on nodejs.

So I wonder if instead of the custom resolvePathFromRelativeConfig function @amosyuen, it wouldn't be better to just remove the isRelative check in resolvePath and resolve all paths.
Doing so shouldn't cause any troubles afaik (do you confirm @fatfisz ?)

[tleunen]

Nvm, it's not safe to run on all paths as is.
It can create a breaking changes.

Let's say I have an alias "App": './app. But then I have a file requiring a local file called ./app, it will actually rename my import as ./App which is not correct in this case :/
Modifying the original alias as "^App/(.+)": "./app/\\1", fixes it, but it requires the end user to update his config :/