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

fix: correctly resolve remapped directories #12373

Merged
merged 15 commits into from
Feb 17, 2022
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
- `[jest-environment-node]` [**BREAKING**] Add default `node` and `node-addon` conditions to `exportConditions` for `node` environment ([#11924](https://github.com/facebook/jest/pull/11924))
- `[@jest/expect]` New module which extends `expect` with `jest-snapshot` matchers ([#12404](https://github.com/facebook/jest/pull/12404), [#12410](https://github.com/facebook/jest/pull/12410), [#12418](https://github.com/facebook/jest/pull/12418))
- `[@jest/expect-utils]` New module exporting utils for `expect` ([#12323](https://github.com/facebook/jest/pull/12323))
- `[jest-resolver]` [**BREAKING**] Add support for `package.json` `exports` ([11961](https://github.com/facebook/jest/pull/11961))
- `[jest-resolve]` [**BREAKING**] Add support for `package.json` `exports` ([#11961](https://github.com/facebook/jest/pull/11961), [#12373](https://github.com/facebook/jest/pull/12373))
- `[jest-resolve, jest-runtime]` Add support for `data:` URI import and mock ([#12392](https://github.com/facebook/jest/pull/12392))
- `[@jest/schemas]` New module for JSON schemas for Jest's config ([#12384](https://github.com/facebook/jest/pull/12384))
- `[jest-worker]` [**BREAKING**] Allow only absolute `workerPath` ([#12343](https://github.com/facebook/jest/pull/12343))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@ describe('Runtime internal module registry', () => {
it('behaves correctly when requiring a module that is used by jest internals', () => {
const fs = require('fs');

// We require from this crazy path so that we can mimick Jest (and it's
// transitive deps) being installed along side a projects deps (e.g. with an
// We require from this crazy path so that we can mimick Jest (and its
// transitive deps) being installed alongside a projects deps (e.g. with an
// NPM3 flat dep tree)
const jestUtil = require('../../../packages/jest-util');
Copy link
Member Author

Choose a reason for hiding this comment

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

I honestly don't understand this test 🙈 Comes from #1572

Copy link
Member Author

Choose a reason for hiding this comment

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

comment above doesn't make much sense, you're not simulating a real environment by using a relative path to get into some module

const jestUtil = require('jest-util');

// If FS is mocked correctly, this folder won't actually be created on the
// filesystem
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 14 additions & 0 deletions packages/jest-resolve/src/__tests__/resolve.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,20 @@ describe('findNodeModule', () => {
path.resolve(conditionsRoot, './node_modules/exports/nestedDefault.js'),
);
});

test('supports separate directory path', () => {
const result = Resolver.findNodeModule('exports/directory/file.js', {
basedir: conditionsRoot,
conditions: [],
});

expect(result).toEqual(
path.resolve(
conditionsRoot,
'./node_modules/exports/some-other-directory/file.js',
),
);
});
});
});

Expand Down
114 changes: 60 additions & 54 deletions packages/jest-resolve/src/defaultResolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,13 @@
* LICENSE file in the root directory of this source tree.
*/

import {isAbsolute} from 'path';
import {dirname, isAbsolute, resolve as pathResolve} from 'path';
import pnpResolver from 'jest-pnp-resolver';
import {sync as resolveSync} from 'resolve';
import {SyncOpts as UpstreamResolveOptions, sync as resolveSync} from 'resolve';
import {
Options as ResolveExportsOptions,
resolve as resolveExports,
} from 'resolve.exports';
import slash = require('slash');
import {
PkgJson,
isDirectory,
Expand All @@ -36,6 +35,9 @@ interface ResolverOptions {
pathFilter?: (pkg: PkgJson, path: string, relativePath: string) => string;
}

type UpstreamResolveOptionsWithConditions = UpstreamResolveOptions &
Pick<ResolverOptions, 'conditions'>;

// https://github.com/facebook/jest/pull/10617
declare global {
namespace NodeJS {
Expand All @@ -55,16 +57,22 @@ export default function defaultResolver(
return pnpResolver(path, options);
}

const result = resolveSync(path, {
const resolveOptions: UpstreamResolveOptionsWithConditions = {
...options,
isDirectory,
isFile,
packageFilter: createPackageFilter(path, options.packageFilter),
pathFilter: createPathFilter(path, options.conditions, options.pathFilter),
preserveSymlinks: false,
readPackageSync,
realpathSync,
});
};

const pathToResolve = getPathInModule(path, resolveOptions);

const result =
// if `getPathInModule` doesn't change the path, attempt to resolve it
pathToResolve === path
? resolveSync(pathToResolve, resolveOptions)
: pathToResolve;

// Dereference symlinks to ensure we don't create a separate
// module instance depending on how it was referenced.
Expand All @@ -79,67 +87,65 @@ function readPackageSync(_: unknown, file: string): PkgJson {
return readPackageCached(file);
}

function createPackageFilter(
originalPath: string,
userFilter?: ResolverOptions['packageFilter'],
): ResolverOptions['packageFilter'] {
if (shouldIgnoreRequestForExports(originalPath)) {
return userFilter;
function getPathInModule(
path: string,
options: UpstreamResolveOptionsWithConditions,
): string {
if (shouldIgnoreRequestForExports(path)) {
return path;
}

return function packageFilter(pkg, ...rest) {
let filteredPkg = pkg;
const segments = path.split('/');

if (userFilter) {
filteredPkg = userFilter(filteredPkg, ...rest);
}
let moduleName = segments.shift();

if (filteredPkg.exports == null) {
return filteredPkg;
if (moduleName) {
// TODO: handle `#` here: https://github.com/facebook/jest/issues/12270
if (moduleName.startsWith('@')) {
moduleName = `${moduleName}/${segments.shift()}`;
}

return {
...filteredPkg,
// remove `main` so `resolve` doesn't look at it and confuse the `.`
// loading in `pathFilter`
main: undefined,
};
};
}
let packageJsonPath = '';

function createPathFilter(
originalPath: string,
conditions?: Array<string>,
userFilter?: ResolverOptions['pathFilter'],
): ResolverOptions['pathFilter'] {
if (shouldIgnoreRequestForExports(originalPath)) {
return userFilter;
}
try {
packageJsonPath = resolveSync(`${moduleName}/package.json`, options);
} catch {
// ignore if package.json cannot be found
}

const options: ResolveExportsOptions = conditions
? {conditions, unsafe: true}
: // no conditions were passed - let's assume this is Jest internal and it should be `require`
{browser: false, require: true};
if (packageJsonPath && isFile(packageJsonPath)) {
const pkg = readPackageCached(packageJsonPath);

return function pathFilter(pkg, path, relativePath, ...rest) {
let pathToUse = relativePath;
if (pkg.exports) {
// we need to make sure resolve ignores `main`
delete pkg.main;

if (userFilter) {
pathToUse = userFilter(pkg, path, relativePath, ...rest);
}
const subpath = segments.join('/') || '.';

if (pkg.exports == null) {
return pathToUse;
}
const resolved = resolveExports(
pkg,
subpath,
createResolveOptions(options.conditions),
);

// this `index` thing can backfire, but `resolve` adds it: https://github.com/browserify/resolve/blob/f1b51848ecb7f56f77bfb823511d032489a13eab/lib/sync.js#L192
const isRootRequire =
pathToUse === 'index' && !originalPath.endsWith('/index');
// TODO: should we throw if not?
if (resolved) {
return pathResolve(dirname(packageJsonPath), resolved);
}
}
}
}

const newPath = isRootRequire ? '.' : slash(pathToUse);
return path;
}

return resolveExports(pkg, newPath, options) || pathToUse;
};
function createResolveOptions(
conditions: Array<string> | undefined,
): ResolveExportsOptions {
return conditions
? {conditions, unsafe: true}
: // no conditions were passed - let's assume this is Jest internal and it should be `require`
{browser: false, require: true};
}

// if it's a relative import or an absolute path, exports are ignored
Expand Down
6 changes: 3 additions & 3 deletions packages/jest-worker/src/__tests__/leak-integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {tmpdir} from 'os';
import {join} from 'path';
import {writeFileSync} from 'graceful-fs';
import LeakDetector from 'jest-leak-detector';
import {Worker} from '../..';
import {Worker} from '../../build/index';

let workerFile!: string;
beforeAll(() => {
Expand All @@ -30,10 +30,10 @@ afterEach(async () => {

it('does not retain arguments after a task finished', async () => {
let leakDetector!: LeakDetector;
await new Promise(resolve => {
await new Promise((resolve, reject) => {
const obj = {};
leakDetector = new LeakDetector(obj);
(worker as any).fn(obj).then(resolve);
(worker as any).fn(obj).then(resolve, reject);
});

expect(await leakDetector.isLeaking()).toBe(false);
Expand Down