-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(lambda-nodejs): use docker instead of npm for parcel-bundler #7169
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
Changes from 2 commits
ea0e032
3f9db92
4490601
157281e
74383e2
e744f9c
e1428ae
9a5cb5c
370778d
d36a005
23ffd92
459e587
b64686d
5fc915d
9842450
34afc14
6f8fd3c
0404baa
ebb464f
5f9d9ab
eaf48b2
86ee7d2
2f63cab
a78b7e0
24d9e8d
5fa1e63
c7852e9
9f85d87
d3c79d4
feadb67
711b58e
8de8246
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,5 @@ | ||||||||
| FROM node:13.8.0-alpine3.11 | ||||||||
|
||||||||
| FROM node:13.8.0-alpine3.11 | |
| ARG VERSION=13.8.0-alpine3.11 | |
| FROM node:$VERSION |
With a prop allowing to customize the version? and the default either in the Dockerfile as above or in the .ts file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we expose this build arg as an optional prop in NodejsFunctionProps and then BuilderOptions? There's a risk of being stucked with a parcel-bundler version that could be incompatible with the fixed node/alpine version of the Dockerfile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expose nodeDockerTag in NodejsFunctionProps and pass it to the Builder and we're done here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
AlexCheema marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,33 +1,22 @@ | ||
| import { spawnSync } from 'child_process'; | ||
| import * as fs from 'fs'; | ||
| import * as path from 'path'; | ||
| import { Builder } from '../lib/builder'; | ||
|
|
||
| let parcelPkgPath: string; | ||
| let parcelPkg: Buffer; | ||
| beforeAll(() => { | ||
| parcelPkgPath = require.resolve('parcel-bundler/package.json'); | ||
| parcelPkg = fs.readFileSync(parcelPkgPath); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| fs.writeFileSync(parcelPkgPath, parcelPkg); | ||
| }); | ||
|
|
||
| jest.mock('child_process', () => ({ | ||
| spawnSync: jest.fn((_cmd: string, args: string[]) => { | ||
| if (args[1] === 'error') { | ||
| if (args.includes('/entry/error')) { | ||
| return { error: 'parcel-error' }; | ||
| } | ||
|
|
||
| if (args[1] === 'status') { | ||
| if (args.includes('/entry/status')) { | ||
| return { status: 1, stdout: Buffer.from('status-error') }; | ||
| } | ||
|
|
||
| return { error: null, status: 0 }; | ||
| }) | ||
| })); | ||
|
|
||
| test('calls parcel with the correct args', () => { | ||
| test('calls docker with the correct args', () => { | ||
| const builder = new Builder({ | ||
| entry: 'entry', | ||
| global: 'handler', | ||
|
|
@@ -36,18 +25,29 @@ test('calls parcel with the correct args', () => { | |
| }); | ||
| builder.build(); | ||
|
|
||
| expect(spawnSync).toHaveBeenCalledWith(expect.stringContaining('parcel-bundler'), expect.arrayContaining([ | ||
| 'build', 'entry', | ||
| '--out-dir', 'out-dir', | ||
| // docker build | ||
| expect(spawnSync).toHaveBeenNthCalledWith(1, 'docker', [ | ||
| 'build', '-t', 'parcel-bundler', path.join(__dirname, '../parcel-bundler') | ||
| ]); | ||
|
|
||
| // docker run | ||
| expect(spawnSync).toHaveBeenNthCalledWith(2, 'docker', [ | ||
| 'run', '--rm', | ||
| '-v', `${path.join(__dirname, '..')}:/entry`, | ||
| '-v', `${path.join(__dirname, '../out-dir')}:/out`, | ||
| '-v', `${path.join(__dirname, '../cache-dir')}:/cache`, | ||
| 'parcel-bundler', | ||
| 'parcel', 'build', '/entry/entry', | ||
| '--out-dir', '/out', | ||
| '--out-file', 'index.js', | ||
| '--global', 'handler', | ||
| '--target', 'node', | ||
| '--bundle-node-modules', | ||
| '--log-level', '2', | ||
| '--no-minify', | ||
| '--no-source-maps', | ||
| '--cache-dir', 'cache-dir' | ||
| ])); | ||
| '--cache-dir', '/cache' | ||
| ]); | ||
| }); | ||
|
|
||
| test('throws in case of error', () => { | ||
|
|
@@ -67,15 +67,3 @@ test('throws if status is not 0', () => { | |
| }); | ||
| expect(() => builder.build()).toThrow('status-error'); | ||
| }); | ||
|
|
||
| test('throws when parcel-bundler is not 1.x', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a test for when
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| fs.writeFileSync(parcelPkgPath, JSON.stringify({ | ||
| ...JSON.parse(parcelPkg.toString()), | ||
| version: '2.3.4' | ||
| })); | ||
| expect(() => new Builder({ | ||
| entry: 'entry', | ||
| global: 'handler', | ||
| outDir: 'out-dur' | ||
| })).toThrow(/This module has a peer dependency on parcel-bundler v1.x. Got v2.3.4./); | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.