Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
501b839
Join aliases and regular expressions (no change in behavior)
fatfisz Apr 9, 2017
fa481cd
Always localize aliased paths (also reg exp)
fatfisz Apr 9, 2017
3e15eec
Move the aliasedSourceFile processing outside the loop
fatfisz Apr 9, 2017
b42ceb2
Print a deprecation warning when npm: is in the config
fatfisz Apr 9, 2017
c55a2d0
Try to resolve aliased paths using the root and cwd
fatfisz Apr 9, 2017
fda055b
Add a test for root and alias resolving order
fatfisz Apr 9, 2017
024dcb1
Remove support for npm: completely
fatfisz Apr 9, 2017
81d528d
Do only one pass over the aliasKeys array
fatfisz Apr 9, 2017
da1fcd9
Fix a typo
fatfisz Apr 10, 2017
1b4f87f
Don't resolve aliased paths again
fatfisz Apr 10, 2017
c59bcee
Restore the test for the double alias application
fatfisz Apr 10, 2017
040bb1c
Remove a useless test (alias is not resolved if it's a package name)
fatfisz Apr 10, 2017
7c71d3e
Make most alias paths into relative ones
fatfisz Apr 10, 2017
ce36607
Remove obsolete tests
fatfisz Apr 10, 2017
5b1b900
Warn if the package cannot be resolved after aliasing
fatfisz Apr 10, 2017
3caaaa1
Fix paths in the tests.
fatfisz Apr 10, 2017
c0baaa0
Use opts.extensions for resolving the aliased module path
fatfisz Apr 13, 2017
66bc9c6
Add a missing test for the production environment
fatfisz Apr 13, 2017
dc01d6d
Al aliases should be tried in the enumeration order
fatfisz Apr 13, 2017
8e55566
Merge branch 'beta' into resolve-alias-using-root
fatfisz Apr 13, 2017
647ce41
Add a useful test
fatfisz Apr 13, 2017
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
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@
"jest": {
"testRegex": "/test/.*\\.test\\.js$",
"collectCoverageFrom": [
"src/**/*.js"
"src/**/*.js",
"!src/log.js"
]
},
"greenkeeper": {
Expand Down
50 changes: 17 additions & 33 deletions src/getRealPath.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,40 +43,10 @@ function getRealPathFromRootConfig(sourcePath, currentFile, opts) {
)));
}

function getRealPathFromAliasConfig(sourcePath, currentFile, { alias, cwd }) {
const moduleSplit = sourcePath.split('/');
let aliasPath;

while (moduleSplit.length) {
const m = moduleSplit.join('/');
if ({}.hasOwnProperty.call(alias, m)) {
aliasPath = alias[m];
break;
}
moduleSplit.pop();
}

// no alias mapping found
if (!aliasPath) {
return null;
}

// remove legacy "npm:" prefix for npm packages
aliasPath = aliasPath.replace(/^(npm:)/, '');
const newPath = sourcePath.replace(moduleSplit.join('/'), aliasPath);

// alias to npm module don't need relative mapping
if (aliasPath[0] !== '.') {
return newPath;
}

return toLocalPath(toPosixPath(mapToRelative(cwd, currentFile, newPath)));
}

function getRealPathFromRegExpConfig(sourcePath, currentFile, { regExps }) {
function getRealPathFromAliasConfig(sourcePath, currentFile, opts) {
let aliasedSourceFile;

regExps.find(([regExp, substitute]) => {
opts.alias.find(([regExp, substitute]) => {
const execResult = regExp.exec(sourcePath);

if (execResult === null) {
Expand All @@ -87,13 +57,27 @@ function getRealPathFromRegExpConfig(sourcePath, currentFile, { regExps }) {
return true;
});

if (!aliasedSourceFile) {
return null;
}

if (aliasedSourceFile[0] === '.') {
return toLocalPath(toPosixPath(
mapToRelative(opts.cwd, currentFile, aliasedSourceFile)),
);
}

const realPathFromRoot = getRealPathFromRootConfig(aliasedSourceFile, currentFile, opts);
if (realPathFromRoot) {
return realPathFromRoot;
}

return aliasedSourceFile;
}

const resolvers = [
getRealPathFromRootConfig,
getRealPathFromAliasConfig,
getRealPathFromRegExpConfig,
];

export default function getRealPath(sourcePath, { file, opts }) {
Expand Down
7 changes: 7 additions & 0 deletions src/log.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// This module exists only for abstracting logging away and making testing easier

// eslint-disable-next-line import/prefer-default-export
export function warn(...args) {
// eslint-disable-next-line no-console
console.warn(...args);
}
52 changes: 35 additions & 17 deletions src/normalizeOptions.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import path from 'path';

import findBabelConfig from 'find-babel-config';
import glob from 'glob';
import { warn } from './log';


const defaultExtensions = ['.js', '.jsx', '.es', '.es6'];
Expand Down Expand Up @@ -51,29 +52,46 @@ function normalizeRoot(opts) {
}
}

function normalizeAlias(opts) {
opts.regExps = [];
function getAliasPair(key, value) {
const parts = value.split('\\\\');

function substitute(execResult) {
return parts
.map(part =>
part.replace(/\\\d+/g, number => execResult[number.slice(1)] || ''),
)
.join('\\');
}

return [new RegExp(key), substitute];
}

function normalizeAlias(opts) {
if (opts.alias) {
Object.keys(opts.alias)
.filter(isRegExp)
.forEach((key) => {
const parts = opts.alias[key].split('\\\\');

function substitute(execResult) {
return parts
.map(part =>
part.replace(/\\\d+/g, number => execResult[number.slice(1)] || ''),
)
.join('\\');
const { alias } = opts;
const aliasKeys = Object.keys(alias);
let warnedAboutNpmPrefix = false;

const nonRegExpAliases = aliasKeys
.filter(key => !isRegExp(key))
.map((key) => {
let value = alias[key];

if (value.startsWith('npm:') && !warnedAboutNpmPrefix) {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Lets drop this for good :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If you say so :) I'm just worried about breaking some more configs :D

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

It will be a 3.0 so no problem removing the deprecated things :) Especially since there's a way to do it without using the prefix.

warnedAboutNpmPrefix = true;
warn('The "npm:" prefix in an alias is deprecated and will be removed in the next major version release.');
value = value.slice('npm:'.length);
}
return getAliasPair(`^${key}((?:/|).*)`, `${value}\\1`);
});

opts.regExps.push([new RegExp(key), substitute]);
const regExpAliases = aliasKeys
.filter(isRegExp)
.map(key => getAliasPair(key, alias[key]));
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Would be best to do both case in the same map loop.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, will join those cases.


delete opts.alias[key];
});
opts.alias = [...nonRegExpAliases, ...regExpAliases];
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

What do you think of this?

opts.alias = aliasKeys.reduce((acc, key) => {
  acc.push(
    isRegExp(key) 
      ? getAliasPair(key, alias[key])
      : getAliasPair(`^${key}((?:/|).*)`, `${alias[key]}\\1`)
  );
  return acc;
}, []);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wanted to preserve the order: standard aliases before regexps. At this point I think it could go either way, as the order should be taken from the object.

Note: according to the spec, the order of enumerating the object keys is not guaranteed to be the same as in the literal, so in the future it may be needed to allow an array-type declaration. We're currently relying on V8.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Indeed, one or the other, we'll have an issue if a user expects the order to be kept. With an object, it's not guaranteed as you say. We'll figure out a way later if really needed. For now, it doesn't seem like users need it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll go with the single array then.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

(btw. map is even better than forEach here, will use that instead)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Indeed.

} else {
opts.alias = {};
opts.alias = [];
}
}

Expand Down
87 changes: 72 additions & 15 deletions test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,28 @@ describe('module-resolver', () => {
);
});
});

describe('root and alias', () => {
const rootTransformerOpts = {
babelrc: false,
plugins: [
[plugin, {
root: './test/testproject/src',
alias: {
constants: 'constants/actions',
},
}],
],
};

it('should resolve the path using root first and alias otherwise', () => {
testWithImport(
'constants',
'./test/testproject/src/constants',
rootTransformerOpts,
);
});
});
});

describe('alias', () => {
Expand All @@ -221,7 +243,6 @@ describe('module-resolver', () => {
components: './test/testproject/src/components',
'~': './test/testproject/src',
'awesome/components': './test/testproject/src/components',
abstract: 'npm:concrete',
underscore: 'lodash',
prefix: 'prefix/lib',
'^@namespace/foo-(.+)': 'packages/\\1',
Expand Down Expand Up @@ -306,12 +327,48 @@ describe('module-resolver', () => {
});
});

it('(legacy) should support aliasing a node module with "npm:"', () => {
testWithImport(
'abstract/thing',
'concrete/thing',
aliasTransformerOpts,
);
describe('should handle the deprecated "npm:" prefix', () => {
const mockWarn = jest.fn();
jest.mock('../src/log', () => ({
warn: mockWarn,
}));
jest.resetModules();
const pluginWithMock = require.requireActual('../src').default;

const depreactedAliasTransformerOpts = {
plugins: [
[pluginWithMock, {
alias: {
abstract: 'npm:concrete',
second: 'npm:another',
nonNpmPrefix: 'normal',
},
}],
],
};

beforeEach(() => {
mockWarn.mockClear();
});

it('should print a warning once if "npm:" is encountered in the config', () => {
testWithImport(
'nonNpmPrefix/thing',
'normal/thing',
depreactedAliasTransformerOpts,
);

expect(mockWarn.mock.calls.length).toBe(1);
expect(mockWarn).toBeCalledWith('The "npm:" prefix in an alias is deprecated and will be removed in the next major version release.');
});

it('should strip the "npm:" prefix from the resolved path', () => {
testWithImport(
'abstract/thing',
'concrete/thing',
depreactedAliasTransformerOpts,
);
});
});

it('should support aliasing a node modules', () => {
Expand Down Expand Up @@ -433,34 +490,34 @@ describe('module-resolver', () => {
);
});

it('should alias the relative path while ignoring cwd and root', () => {
it('should alias the relative path while honoring cwd and ignoring root', () => {
testWithImport(
'constantsRelative/actions',
'./test/constants/actions',
transformerOpts,
);
});

it('should alias the non-relative path while ignoring cwd and root', () => {
it('should alias the non-relative path while honoring cwd and root', () => {
testWithImport(
'constantsNonRelative/actions',
'constants/actions',
'./test/testproject/src/constants/actions',
transformerOpts,
);
});

it('should alias the relative path while ignoring cwd and root', () => {
it('should alias the relative path while honoring cwd and ignoring root', () => {
testWithImport(
'constantsRegExpRelative/actions',
'./constants/actions',
'./test/constants/actions',
transformerOpts,
);
});

it('should alias the non-relative path while ignoring cwd and root', () => {
it('should alias the non-relative path while honoring cwd and root', () => {
testWithImport(
'constantsNonRelative/actions',
'constants/actions',
'./test/testproject/src/constants/actions',
transformerOpts,
);
});
Expand Down Expand Up @@ -533,7 +590,7 @@ describe('module-resolver', () => {
it('should alias the sub file path', () => {
testWithImport(
'actions',
'constants/actions',
'./constants/actions',
transformerOpts,
);
});
Expand Down