Skip to content

Commit

Permalink
fix(lambda-nodejs): local parcel not detected (#10268)
Browse files Browse the repository at this point in the history 
Fix Parcel detection for non JS/TS CDK projects. For those projects the
module `@aws-cdk/aws-lambda-nodejs` is not installed in a
`node_modules` folder inside the project.

Change the detection logic to `require.resolve` from the project root.

Also in this fix: ensure that the Parcel version that is run inside the
container is the one installed at `/`. Previously, if an incorrect
version of Parcel was detected bundling would happen in a container as
expected but with the incorrect version because project root is mounted
at `/asset-input` and in this case it contains the incorrect Parcel
version at `/asset-input/node_modules`. Again change the
`require.resolve` paths to avoid this.

Addresses #10123 (not sure yet if it closes it)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
@jogold
jogold authored Sep 21, 2020
1 parent 114b093 commit 457fab8
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 37 deletions.
38 changes: 27 additions & 11 deletions packages/@aws-cdk/aws-lambda-nodejs/lib/bundlers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,32 +24,36 @@ interface LocalBundlerProps extends BundlerProps {
* Local Parcel bundler
*/
export class LocalBundler implements cdk.ILocalBundling {
public static get runsLocally(): boolean {
if (LocalBundler._runsLocally !== undefined) {
return LocalBundler._runsLocally;
public static runsLocally(resolvePath: string): boolean {
if (LocalBundler._runsLocally[resolvePath] !== undefined) {
return LocalBundler._runsLocally[resolvePath];
}
if (os.platform() === 'win32') { // TODO: add Windows support
return false;
}
try {
const parcel = spawnSync(require.resolve('parcel'), ['--version']);
const parcel = spawnSync(require.resolve('parcel', { paths: [resolvePath] }), ['--version']);
const version = parcel.stdout.toString().trim();
LocalBundler._runsLocally = new RegExp(`^${PARCEL_VERSION}`).test(version); // Cache result to avoid unnecessary spawns
if (!LocalBundler._runsLocally) {
LocalBundler._runsLocally[resolvePath] = new RegExp(`^${PARCEL_VERSION}`).test(version); // Cache result to avoid unnecessary spawns
if (!LocalBundler._runsLocally[resolvePath]) {
process.stderr.write(`Incorrect parcel version detected: ${version} <> ${PARCEL_VERSION}. Switching to Docker bundling.\n`);
}
return LocalBundler._runsLocally;
} catch {
return LocalBundler._runsLocally[resolvePath];
} catch (err) {
return false;
}
}

public static _runsLocally?: boolean; // public for testing purposes
public static clearRunsLocallyCache(): void { // for tests
LocalBundler._runsLocally = {};
}

private static _runsLocally: { [key: string]: boolean } = {};

constructor(private readonly props: LocalBundlerProps) {}

public tryBundle(outputDir: string) {
if (!LocalBundler.runsLocally) {
if (!LocalBundler.runsLocally(this.props.projectRoot)) {
return false;
}

Expand All @@ -61,6 +65,7 @@ export class LocalBundler implements cdk.ILocalBundling {
dependencies: this.props.dependencies,
installer: this.props.installer,
lockFile: this.props.lockFile,
bundlingEnvironment: BundlingEnvironment.LOCAL,
});

exec('bash', ['-c', localCommand], {
Expand Down Expand Up @@ -109,6 +114,7 @@ export class DockerBundler {
installer: props.installer,
lockFile: props.lockFile,
dependencies: props.dependencies,
bundlingEnvironment: BundlingEnvironment.DOCKER,
});

this.bundlingOptions = {
Expand All @@ -122,6 +128,12 @@ export class DockerBundler {

interface BundlingCommandOptions extends LocalBundlerProps {
outputDir: string;
bundlingEnvironment: BundlingEnvironment;
}

enum BundlingEnvironment {
DOCKER = 'docker',
LOCAL = 'local',
}

/**
Expand All @@ -130,9 +142,13 @@ interface BundlingCommandOptions extends LocalBundlerProps {
function createBundlingCommand(options: BundlingCommandOptions): string {
const entryPath = path.join(options.projectRoot, options.relativeEntryPath);
const distFile = path.basename(options.relativeEntryPath).replace(/\.(jsx|tsx?)$/, '.js');
const parcelResolvePath = options.bundlingEnvironment === BundlingEnvironment.DOCKER
? '/' // Force using parcel installed at / in the image
: entryPath; // Look up starting from entry path

const parcelCommand: string = chain([
[
'$(node -p "require.resolve(\'parcel\')")', // Parcel is not globally installed, find its "bin"
`$(node -p "require.resolve(\'parcel\', { paths: ['${parcelResolvePath}'] })")`, // Parcel is not globally installed, find its "bin"
'build', entryPath.replace(/\\/g, '/'), // Always use POSIX paths in the container
'--target', 'cdk-lambda',
'--dist-dir', options.outputDir, // Output bundle in outputDir (will have the same name as the entry)
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-lambda-nodejs/lib/bundling.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ export class Bundling {
cacheDir: options.cacheDir,
environment: options.parcelEnvironment,
bundlingDockerImage: options.bundlingDockerImage,
buildImage: !LocalBundler.runsLocally || options.forceDockerBundling,
buildImage: !LocalBundler.runsLocally(projectRoot) || options.forceDockerBundling, // build image only if we can't run locally
buildArgs: options.buildArgs,
parcelVersion: options.parcelVersion,
dependencies,
Expand Down
38 changes: 13 additions & 25 deletions packages/@aws-cdk/aws-lambda-nodejs/test/bundling.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ const fromAssetMock = jest.spyOn(BundlingDockerImage, 'fromAsset');
let findUpMock: jest.SpyInstance;
beforeEach(() => {
jest.clearAllMocks();
LocalBundler.clearRunsLocallyCache();
findUpMock = jest.spyOn(util, 'findUp').mockImplementation((name: string, directory) => {
if (name === 'package.json') {
return path.join(__dirname, '..');
Expand Down Expand Up @@ -53,7 +54,7 @@ test('Parcel bundling', () => {
command: [
'bash', '-c',
[
'$(node -p "require.resolve(\'parcel\')") build /asset-input/folder/entry.ts --target cdk-lambda --dist-dir /asset-output --no-autoinstall --no-scope-hoist --cache-dir /asset-input/cache-dir',
'$(node -p "require.resolve(\'parcel\', { paths: [\'/\'] })") build /asset-input/folder/entry.ts --target cdk-lambda --dist-dir /asset-output --no-autoinstall --no-scope-hoist --cache-dir /asset-input/cache-dir',
'mv /asset-output/entry.js /asset-output/index.js',
].join(' && '),
],
Expand Down Expand Up @@ -96,7 +97,7 @@ test('Parcel bundling with handler named index.ts', () => {
bundling: expect.objectContaining({
command: [
'bash', '-c',
'$(node -p "require.resolve(\'parcel\')") build /asset-input/folder/index.ts --target cdk-lambda --dist-dir /asset-output --no-autoinstall --no-scope-hoist',
'$(node -p "require.resolve(\'parcel\', { paths: [\'/\'] })") build /asset-input/folder/index.ts --target cdk-lambda --dist-dir /asset-output --no-autoinstall --no-scope-hoist',
],
}),
});
Expand All @@ -116,7 +117,7 @@ test('Parcel bundling with tsx handler', () => {
command: [
'bash', '-c',
[
'$(node -p "require.resolve(\'parcel\')") build /asset-input/folder/handler.tsx --target cdk-lambda --dist-dir /asset-output --no-autoinstall --no-scope-hoist',
'$(node -p "require.resolve(\'parcel\', { paths: [\'/\'] })") build /asset-input/folder/handler.tsx --target cdk-lambda --dist-dir /asset-output --no-autoinstall --no-scope-hoist',
'mv /asset-output/handler.js /asset-output/index.js',
].join(' && '),
],
Expand Down Expand Up @@ -156,7 +157,7 @@ test('Parcel bundling with externals and dependencies', () => {
command: [
'bash', '-c',
[
'$(node -p "require.resolve(\'parcel\')") build /asset-input/folder/entry.ts --target cdk-lambda --dist-dir /asset-output --no-autoinstall --no-scope-hoist',
'$(node -p "require.resolve(\'parcel\', { paths: [\'/\'] })") build /asset-input/folder/entry.ts --target cdk-lambda --dist-dir /asset-output --no-autoinstall --no-scope-hoist',
'mv /asset-output/entry.js /asset-output/index.js',
`echo \'{\"dependencies\":{\"delay\":\"${delayVersion}\"}}\' > /asset-output/package.json`,
'cd /asset-output',
Expand Down Expand Up @@ -229,15 +230,15 @@ test('Local bundling', () => {
const spawnSyncMock = jest.spyOn(child_process, 'spawnSync').mockReturnValue({
status: 0,
stderr: Buffer.from('stderr'),
stdout: Buffer.from('stdout'),
stdout: Buffer.from('2.0.0-beta.1'),
pid: 123,
output: ['stdout', 'stderr'],
signal: null,
});

const bundler = new LocalBundler({
installer: Installer.NPM,
projectRoot: '/project',
projectRoot: __dirname,
relativeEntryPath: 'folder/entry.ts',
dependencies: {
dep: 'version',
Expand All @@ -251,20 +252,11 @@ test('Local bundling', () => {
bundler.tryBundle('/outdir');

expect(spawnSyncMock).toHaveBeenCalledWith(
'bash', [
'-c',
[
'$(node -p \"require.resolve(\'parcel\')\") build /project/folder/entry.ts --target cdk-lambda --dist-dir /outdir --no-autoinstall --no-scope-hoist',
'mv /outdir/entry.js /outdir/index.js',
'echo \'{\"dependencies\":{\"dep\":\"version\"}}\' > /outdir/package.json',
'cp /project/package-lock.json /outdir/package-lock.json',
'cd /outdir',
'npm install',
].join(' && '),
],
'bash',
expect.arrayContaining(['-c', expect.stringContaining(__dirname)]),
expect.objectContaining({
env: expect.objectContaining({ KEY: 'value' }),
cwd: '/project/folder',
cwd: expect.stringContaining(path.join(__dirname, 'folder')),
}),
);

Expand All @@ -273,8 +265,6 @@ test('Local bundling', () => {
});

test('LocalBundler.runsLocally checks parcel version and caches results', () => {
LocalBundler._runsLocally = undefined;

const spawnSyncMock = jest.spyOn(child_process, 'spawnSync').mockReturnValue({
status: 0,
stderr: Buffer.from('stderr'),
Expand All @@ -284,15 +274,13 @@ test('LocalBundler.runsLocally checks parcel version and caches results', () =>
signal: null,
});

expect(LocalBundler.runsLocally).toBe(true);
expect(LocalBundler.runsLocally).toBe(true);
expect(LocalBundler.runsLocally(__dirname)).toBe(true);
expect(LocalBundler.runsLocally(__dirname)).toBe(true);
expect(spawnSyncMock).toHaveBeenCalledTimes(1);
expect(spawnSyncMock).toHaveBeenCalledWith(expect.stringContaining('parcel'), ['--version']);
});

test('LocalBundler.runsLocally with incorrect parcel version', () => {
LocalBundler._runsLocally = undefined;

jest.spyOn(child_process, 'spawnSync').mockReturnValue({
status: 0,
stderr: Buffer.from('stderr'),
Expand All @@ -302,7 +290,7 @@ test('LocalBundler.runsLocally with incorrect parcel version', () => {
signal: null,
});

expect(LocalBundler.runsLocally).toBe(false);
expect(LocalBundler.runsLocally(__dirname)).toBe(false);
});

test('Project root detection', () => {
Expand Down

0 comments on commit 457fab8

Please sign in to comment.