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 mishandling of package lookup cache #225

Merged
merged 2 commits into from
Jun 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 8 additions & 5 deletions ember-scoped-css/src/build/babel-plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,22 @@ import {
} from '../lib/path/utils.js';
import rewriteHbs from '../lib/rewriteHbs.js';

function _isRelevantFile(state) {
function _isRelevantFile(state, cwd) {
let fileName = state.file.opts.filename;
let additionalRoots = state.opts?.additionalRoots;

return isRelevantFile(fileName, additionalRoots);
return isRelevantFile(fileName, {
additionalRoots,
cwd,
});
}

/**
* @param {any} env - babel plugin env, env.types is most commonly used (esp in TS)
* @param {object} options - the options for scoped-css -- this is also available in each visitor's state.opts
* @param {string} workingDirectory
*/
export default (/* env, options, workingDirectory */) => {
export default (env, options, workingDirectory) => {
/**
* - This can receive the intermediate output of the old REGEX-based <template> transform:
* ```
Expand All @@ -43,7 +46,7 @@ export default (/* env, options, workingDirectory */) => {
return {
visitor: {
ImportDeclaration(path, state) {
if (!_isRelevantFile(state)) {
if (!_isRelevantFile(state, workingDirectory)) {
return;
}

Expand Down Expand Up @@ -79,7 +82,7 @@ export default (/* env, options, workingDirectory */) => {
}
},
CallExpression(path, state) {
if (!_isRelevantFile(state)) {
if (!_isRelevantFile(state, workingDirectory)) {
return;
}

Expand Down
1 change: 1 addition & 0 deletions ember-scoped-css/src/build/scoped-css-unplugin.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { existsSync } from 'node:fs';
import { readFile } from 'node:fs/promises';
import path from 'node:path';
import process from 'node:process';

import { createUnplugin } from 'unplugin';

Expand Down
5 changes: 4 additions & 1 deletion ember-scoped-css/src/build/template-plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,10 @@ export function createPlugin(config) {
* @param {ASTPluginEnvironment} env
*/
return function scopedCss(env) {
let isRelevant = isRelevantFile(env.filename, config.additionalRoots);
let isRelevant = isRelevantFile(env.filename, {
additionalRoots: config.additionalRoots,
cwd: process.cwd(),
});

if (!isRelevant) {
return noopPlugin;
Expand Down
26 changes: 7 additions & 19 deletions ember-scoped-css/src/lib/path/utils.isRelevantFile.test.ts
Original file line number Diff line number Diff line change
@@ -1,73 +1,61 @@
import path from 'node:path';

import { describe, expect, it, vi } from 'vitest';
import { describe, expect, it } from 'vitest';

import { isRelevantFile } from './utils.js';
import { paths } from './utils.paths.test.js';

describe('isRelevantFile()', () => {
describe('the file is relevant', () => {
it('for in-project files', () => {
vi.spyOn(process, 'cwd').mockReturnValue(paths.embroiderApp);

let file = path.join(paths.embroiderApp, 'app/components/forth.gjs');
let result = isRelevantFile(file);
let result = isRelevantFile(file, { cwd: paths.embroiderApp });

expect(result).toBeTruthy();
});
});

describe('the file is not relevant', () => {
it('for outside-of-project files', () => {
vi.spyOn(process, 'cwd').mockReturnValue(paths.embroiderApp);

let file = path.join(paths.v2Addon, 'dist/components/footer.js');
let result = isRelevantFile(file);
let result = isRelevantFile(file, { cwd: paths.embroiderApp });

expect(result).toBeFalsy();
});

it('for files in node_modules', () => {
vi.spyOn(process, 'cwd').mockReturnValue(paths.embroiderApp);

let file = path.join(
paths.embroiderApp,
'node_modules/ember-resources/dist/index.js',
);
let result = isRelevantFile(file);
let result = isRelevantFile(file, { cwd: paths.embroiderApp });

expect(result).toBeFalsy();
});

it('for files in .embroider/rewritten-packages', () => {
vi.spyOn(process, 'cwd').mockReturnValue(paths.embroiderApp);

let file = path.join(
paths.embroiderApp,
'node_modules/.embroider/rewritten-packages/ember-source/dist/index.js',
);
let result = isRelevantFile(file);
let result = isRelevantFile(file, { cwd: 'does not matter yet' });

expect(result).toBeFalsy();
});

it('for files in .embroider/rewritten-app/assets/tests.js', () => {
vi.spyOn(process, 'cwd').mockReturnValue(paths.embroiderApp);

let file = path.join(
paths.embroiderApp,
'node_modules/.embroider/rewritten-app/assets/tests.js',
);
let result = isRelevantFile(file);
let result = isRelevantFile(file, { cwd: paths.embroiderApp });

expect(result).toBeFalsy();
});

it('for files in tests/', () => {
vi.spyOn(process, 'cwd').mockReturnValue(paths.embroiderApp);

let file = path.join(paths.embroiderApp, 'tests/foo.js');
let result = isRelevantFile(file);
let result = isRelevantFile(file, { cwd: paths.embroiderApp });

expect(result).toBeFalsy();
});
Expand Down
37 changes: 11 additions & 26 deletions ember-scoped-css/src/lib/path/utils.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import assert from 'node:assert';
import fsSync from 'node:fs';
import path from 'node:path';

Expand Down Expand Up @@ -119,10 +120,10 @@ const UNSUPPORTED_DIRECTORIES = new Set(['tests']);
* - URL-absolute path, starting with /
*
* @param {string} fileName
* @param {string[]} [additionalRoots]
* @param {{ additionalRoots?: string[]; cwd: string }} options
* @returns
*/
export function isRelevantFile(fileName, additionalRoots) {
export function isRelevantFile(fileName, { additionalRoots, cwd }) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cwd is now passed in, as it's too hard to mock globals or even modules

if (fileName.startsWith('/@embroider')) return false;
if (IRRELEVANT_PATHS.some((i) => fileName.includes(i))) return false;

Expand Down Expand Up @@ -158,7 +159,9 @@ export function isRelevantFile(fileName, additionalRoots) {
}

let workspace = findWorkspacePath(fileName);
let cwd = process.cwd();

assert(cwd, `cwd was not passed to isRelevantFile`);

let ourWorkspace = findWorkspacePath(cwd);

if (workspace !== ourWorkspace) {
Expand Down Expand Up @@ -262,33 +265,17 @@ export function appPath(sourcePath) {
return `${name}${localPackagerStylePath}`;
}

const CACHE = new Set();

/**
* For a given source path, if we have seen a
* source file within the workspace directory,
* find that workspace directory and return it.
*/
function hasSeen(sourcePath) {
for (let entry of CACHE) {
if (sourcePath.startsWith(entry)) {
return entry;
}
}

// we have not seen this source path yet
return;
}

/**
* Populates the "seen" workspace cache,
* so that we don't hit the file system too often.
*/
export function findWorkspacePath(sourcePath) {
let seen = hasSeen(sourcePath);
let candidatePath = path.join(sourcePath, 'package.json');

const isWorkspace = fsSync.existsSync(candidatePath);

if (seen) {
return seen;
if (isWorkspace) {
return sourcePath;
}

const packageJsonPath = findUp.sync('package.json', {
Expand All @@ -297,8 +284,6 @@ export function findWorkspacePath(sourcePath) {

const workspacePath = path.dirname(packageJsonPath);

CACHE.add(workspacePath);

return workspacePath;
}

Expand Down
Loading