Skip to content

Commit

Permalink
fix(babel): potential race condition in resolveImports
Browse files Browse the repository at this point in the history
  • Loading branch information
Anber committed Sep 5, 2023
1 parent ff339c1 commit a5edbb3
Show file tree
Hide file tree
Showing 12 changed files with 129 additions and 149 deletions.
53 changes: 17 additions & 36 deletions packages/babel/src/plugins/preeval.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ export type PreevalOptions = Pick<
'classNameSlug' | 'displayName' | 'evaluate' | 'features'
> & { eventEmitter: EventEmitter };

const onFinishCallbacks = new WeakMap<object, () => void>();

export default function preeval(
babel: Core,
{ eventEmitter = EventEmitter.dummy, ...options }: PreevalOptions
Expand All @@ -42,51 +40,34 @@ export default function preeval(
const rootScope = file.scope;
this.processors = [];

eventEmitter.pair(
{
method: 'queue:transform:preeval:processTemplate',
},
() => {
file.path.traverse({
Identifier: (p) => {
processTemplateExpression(p, file.opts, options, (processor) => {
processor.dependencies.forEach((dependency) => {
if (dependency.ex.type === 'Identifier') {
addIdentifierToLinariaPreval(rootScope, dependency.ex.name);
}
});

processor.doEvaltimeReplacement();
this.processors.push(processor);
eventEmitter.perf('transform:preeval:processTemplate', () => {
file.path.traverse({
Identifier: (p) => {
processTemplateExpression(p, file.opts, options, (processor) => {
processor.dependencies.forEach((dependency) => {
if (dependency.ex.type === 'Identifier') {
addIdentifierToLinariaPreval(rootScope, dependency.ex.name);
}
});
},
});
}
);

processor.doEvaltimeReplacement();
this.processors.push(processor);
});
},
});
});

if (
isFeatureEnabled(options.features, 'dangerousCodeRemover', filename)
) {
log('start', 'Strip all JSX and browser related stuff');
eventEmitter.pair(
{
method: 'queue:transform:preeval:removeDangerousCode',
},
() => removeDangerousCode(file.path)
eventEmitter.perf('transform:preeval:removeDangerousCode', () =>
removeDangerousCode(file.path)
);
}

onFinishCallbacks.set(
this,
eventEmitter.pair({
method: 'queue:transform:preeval:rest-transformations',
})
);
},
visitor: {},
post(file: BabelFile) {
onFinishCallbacks.get(this)?.();

const log = createCustomDebug('preeval', getFileIdx(file.opts.filename!));

invalidateTraversalCache(file.path);
Expand Down
2 changes: 1 addition & 1 deletion packages/babel/src/transform/Entrypoint.helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ export function loadAndParse(
babelOptions
);

const ast: File = eventEmitter.pair({ method: 'parseFile' }, () =>
const ast: File = eventEmitter.perf('parseFile', () =>
parseFile(babel, name, code, parseConfig)
);

Expand Down
32 changes: 25 additions & 7 deletions packages/babel/src/transform/Entrypoint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,6 @@ function ancestorOrSelf(name: string, parent: ParentEntrypoint) {
return null;
}

type DependencyType = IEntrypointDependency | Promise<IEntrypointDependency>;

export class Entrypoint extends BaseEntrypoint {
public readonly evaluated = false;

Expand Down Expand Up @@ -62,7 +60,11 @@ export class Entrypoint extends BaseEntrypoint {
exports: Record<string | symbol, unknown> | undefined,
evaluatedOnly: string[],
loadedAndParsed?: IEntrypointCode | IIgnoredEntrypoint,
protected readonly dependencies = new Map<string, DependencyType>(),
protected readonly resolveTasks = new Map<
string,
Promise<IEntrypointDependency>
>(),
protected readonly dependencies = new Map<string, IEntrypointDependency>(),
generation = 1
) {
super(services, evaluatedOnly, exports, generation, name, only, parent);
Expand Down Expand Up @@ -150,7 +152,7 @@ export class Entrypoint extends BaseEntrypoint {
pluginOptions: StrictOptions
): Entrypoint | 'loop' {
const { cache, eventEmitter } = services;
return eventEmitter.pair({ method: 'createEntrypoint' }, () => {
return eventEmitter.perf('createEntrypoint', () => {
const [status, entrypoint] = Entrypoint.innerCreate(
services,
parent
Expand Down Expand Up @@ -236,6 +238,7 @@ export class Entrypoint extends BaseEntrypoint {
exports,
evaluatedOnly,
undefined,
cached && 'resolveTasks' in cached ? cached.resolveTasks : undefined,
cached && 'dependencies' in cached ? cached.dependencies : undefined,
cached ? cached.generation + 1 : 1
);
Expand All @@ -248,8 +251,16 @@ export class Entrypoint extends BaseEntrypoint {
return ['created', newEntrypoint];
}

public addDependency(name: string, dependency: DependencyType): void {
this.dependencies.set(name, dependency);
public addDependency(dependency: IEntrypointDependency): void {
this.resolveTasks.delete(dependency.source);
this.dependencies.set(dependency.source, dependency);
}

public addResolveTask(
name: string,
dependency: Promise<IEntrypointDependency>
): void {
this.resolveTasks.set(name, dependency);
}

public createAction<
Expand Down Expand Up @@ -313,10 +324,16 @@ export class Entrypoint extends BaseEntrypoint {
);
}

public getDependency(name: string): DependencyType | undefined {
public getDependency(name: string): IEntrypointDependency | undefined {
return this.dependencies.get(name);
}

public getResolveTask(
name: string
): Promise<IEntrypointDependency> | undefined {
return this.resolveTasks.get(name);
}

public hasLinariaMetadata() {
return this.#hasLinariaMetadata;
}
Expand Down Expand Up @@ -356,6 +373,7 @@ export class Entrypoint extends BaseEntrypoint {
this.exports,
this.evaluatedOnly,
this.loadedAndParsed,
this.resolveTasks,
this.dependencies,
this.generation + 1
);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
/* eslint-disable require-yield */
import { enableDebug } from '@linaria/logger';

import type { IEntrypointDependency } from '../../Entrypoint.types';
import {
createEntrypoint,
Expand Down Expand Up @@ -199,8 +197,6 @@ describe('actionRunner', () => {
});

it('should recover', () => {
enableDebug();

const abortController = new AbortController();
abortController.abort();

Expand Down
5 changes: 4 additions & 1 deletion packages/babel/src/transform/generators/processImports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,14 @@ import type { IProcessImportsAction, SyncScenarioForAction } from '../types';
export function* processImports(
this: IProcessImportsAction
): SyncScenarioForAction<IProcessImportsAction> {
for (const { only, resolved } of this.data.resolved) {
for (const dependency of this.data.resolved) {
const { resolved, only } = dependency;
if (!resolved) {
continue;
}

this.entrypoint.addDependency(dependency);

const nextEntrypoint = this.entrypoint.createChild(resolved, only);
if (nextEntrypoint === 'loop' || nextEntrypoint.ignored) {
continue;
Expand Down
42 changes: 15 additions & 27 deletions packages/babel/src/transform/generators/resolveImports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ import type { Entrypoint } from '../Entrypoint';
import { getStack, isSuperSet, mergeOnly } from '../Entrypoint.helpers';
import type { IEntrypointDependency } from '../Entrypoint.types';
import type {
AsyncScenarioForAction,
IResolveImportsAction,
Services,
SyncScenarioForAction,
AsyncScenarioForAction,
} from '../types';

function emitDependency(
Expand Down Expand Up @@ -154,9 +154,18 @@ export async function* asyncResolveImports(
const resolvedImports = await Promise.all<IEntrypointDependency>(
listOfImports.map(([source, importsOnly]) => {
const cached = entrypoint.getDependency(source);
if (cached instanceof Promise) {
if (cached) {
return {
source,
only: mergeOnly(cached.only, importsOnly),
resolved: cached.resolved,
};
}

const task = entrypoint.getResolveTask(source);
if (task) {
// If we have cached task, we need to merge only…
const newTask = cached.then((res) => {
const newTask = task.then((res) => {
if (isSuperSet(res.only, importsOnly)) {
return res;
}
Expand All @@ -166,38 +175,17 @@ export async function* asyncResolveImports(

log('merging imports %o and %o: %o', importsOnly, res.only, merged);

entrypoint.addDependency(source, {
only: merged,
resolved: res.resolved,
source,
});

return { ...res, only: merged };
});

// … and update the cache
entrypoint.addDependency(source, newTask);
entrypoint.addResolveTask(source, newTask);
return newTask;
}

if (cached) {
const merged = {
source,
only: mergeOnly(cached.only, importsOnly),
resolved: cached.resolved,
};

entrypoint.addDependency(source, merged);

return merged;
}

const resolveTask = getResolveTask(source, importsOnly).then((res) => {
entrypoint.addDependency(source, res);
return res;
});
const resolveTask = getResolveTask(source, importsOnly);

entrypoint.addDependency(source, resolveTask);
entrypoint.addResolveTask(source, resolveTask);

return resolveTask;
})
Expand Down
28 changes: 11 additions & 17 deletions packages/babel/src/transform/generators/transform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,19 +90,15 @@ export const prepareCode = (

const { code, evalConfig, evaluator } = loadedAndParsed;

const preevalStageResult = eventEmitter.pair(
{
method: 'queue:transform:preeval',
},
() =>
runPreevalStage(
babel,
evalConfig,
pluginOptions,
code,
originalAst,
eventEmitter
)
const preevalStageResult = eventEmitter.perf('transform:preeval', () =>
runPreevalStage(
babel,
evalConfig,
pluginOptions,
code,
originalAst,
eventEmitter
)
);

const linariaMetadata = getLinariaMetadata(preevalStageResult.metadata);
Expand All @@ -121,10 +117,8 @@ export const prepareCode = (
features: pluginOptions.features,
};

const [, transformedCode, imports] = eventEmitter.pair(
{
method: 'queue:transform:evaluator',
},
const [, transformedCode, imports] = eventEmitter.perf(
'transform:evaluator',
() =>
evaluator(
evalConfig,
Expand Down
26 changes: 21 additions & 5 deletions packages/testkit/src/babel.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,13 @@ import {
Entrypoint,
} from '@linaria/babel-preset';
import { linariaLogger } from '@linaria/logger';
import type { Evaluator, StrictOptions, OnEvent } from '@linaria/utils';
import type {
Evaluator,
StrictOptions,
OnEvent,
OnActionStartArgs,
OnActionFinishArgs,
} from '@linaria/utils';
import { EventEmitter } from '@linaria/utils';

import serializer from './__utils__/linaria-snapshot-serializer';
Expand Down Expand Up @@ -2555,7 +2561,8 @@ describe('strategy shaker', () => {

it('should ignore unused wildcard reexports', async () => {
const onEvent = jest.fn<void, Parameters<OnEvent>>();
const emitter = new EventEmitter(onEvent);
const onAction = jest.fn<number, OnActionStartArgs | OnActionFinishArgs>();
const emitter = new EventEmitter(onEvent, onAction);
const { code, metadata } = await transform(
dedent`
import { css } from "@linaria/core";
Expand Down Expand Up @@ -2737,7 +2744,8 @@ describe('strategy shaker', () => {

it('evaluates chain of reexports', async () => {
const onEvent = jest.fn<void, Parameters<OnEvent>>();
const emitter = new EventEmitter(onEvent);
const onAction = jest.fn<number, OnActionStartArgs | OnActionFinishArgs>();
const emitter = new EventEmitter(onEvent, onAction);

const { code, metadata } = await transform(
dedent`
Expand Down Expand Up @@ -3203,7 +3211,11 @@ describe('strategy shaker', () => {
const cache = new TransformCacheCollection();

const onEvent = jest.fn<void, Parameters<OnEvent>>();
const emitter = new EventEmitter(onEvent);
const onAction = jest.fn<
number,
OnActionStartArgs | OnActionFinishArgs
>();
const emitter = new EventEmitter(onEvent, onAction);

const files = {
'source-1': dedent`
Expand Down Expand Up @@ -3250,7 +3262,11 @@ describe('strategy shaker', () => {
const cache = new TransformCacheCollection();

const onEvent = jest.fn<void, Parameters<OnEvent>>();
const emitter = new EventEmitter(onEvent);
const onAction = jest.fn<
number,
OnActionStartArgs | OnActionFinishArgs
>();
const emitter = new EventEmitter(onEvent, onAction);

const tokens = ['foo', 'bar', 'bar1', 'bar2'];

Expand Down
Loading

0 comments on commit a5edbb3

Please sign in to comment.