Skip to content

Add support for resolving extensions in relative paths #259

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add support for resolving extensions in relative paths #259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions src/resolvePath.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,20 @@ function getRelativePath(sourcePath, currentFile, absFileInRoot, opts) {
return toLocalPath(toPosixPath(relativePath));
}

function resolvePathFromRelativeConfig(sourcePath, currentFile, opts) {
if (!isRelativePath(sourcePath)) {
return null;
}

const absFileInRoot = nodeResolvePath(sourcePath, path.dirname(currentFile), opts.extensions);

if (!absFileInRoot) {
return null;
}

return getRelativePath(sourcePath, currentFile, absFileInRoot, opts);
}

function findPathInRoots(sourcePath, { extensions, root }) {
// Search the source path inside every custom root directory
let resolvedSourceFile;
Expand Down Expand Up @@ -78,15 +92,12 @@ function resolvePathFromAliasConfig(sourcePath, currentFile, opts) {
}

const resolvers = [
resolvePathFromRelativeConfig,
resolvePathFromAliasConfig,
resolvePathFromRootConfig,
];

export default function resolvePath(sourcePath, currentFile, opts) {
if (isRelativePath(sourcePath)) {
Copy link
Owner

Choose a reason for hiding this comment

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

Do we really need to remove this condition?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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

return sourcePath;
}

const normalizedOpts = normalizeOptions(currentFile, opts);

// File param is a relative path from the environment current working directory
Expand Down
52 changes: 30 additions & 22 deletions test/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,14 @@ describe('module-resolver', () => {
rootTransformerOpts,
);
});

it('should resolve the relative file path with a known defined extension', () => {
testWithImport(
'./test/testproject/src/rn/file',
'./test/testproject/src/rn/file.android.js',
rootTransformerOpts,
);
});
});

describe('root and alias', () => {
Expand All @@ -321,6 +329,28 @@ describe('module-resolver', () => {
);
});
});

describe('with the plugin applied twice', () => {
const doubleAliasTransformerOpts = {
babelrc: false,
plugins: [
[plugin, { root: '.' }],
[plugin, {
alias: {
'^@namespace/foo-(.+)': './packages/\\1',
},
}],
],
};

it('should support replacing parts of a path', () => {
testWithImport(
'@namespace/foo-bar',
'./packages/bar',
doubleAliasTransformerOpts,
);
});
});
});

describe('alias', () => {
Expand Down Expand Up @@ -590,28 +620,6 @@ describe('module-resolver', () => {
});
});

describe('with the plugin applied twice', () => {
const doubleAliasTransformerOpts = {
babelrc: false,
plugins: [
[plugin, { root: '.' }],
[plugin, {
alias: {
'^@namespace/foo-(.+)': './packages/\\1',
},
}],
],
};

it('should support replacing parts of a path', () => {
testWithImport(
'@namespace/foo-bar',
'./packages/bar',
doubleAliasTransformerOpts,
);
});
});

describe('missing packages warning', () => {
const mockWarn = jest.fn();
jest.mock('../src/log', () => ({
Expand Down
Empty file added test/testproject/src/rn/file.android.js
View file
Open in desktop
Empty file.