Migrate Deno Bundle to ESbuild #71
Conversation
this is why I hate comments, they get out of sync
Codecov Report
@@ Coverage Diff @@
## main #71 +/- ##
==========================================
+ Coverage 75.20% 77.24% +2.04%
==========================================
Files 12 16 +4
Lines 746 813 +67
Branches 104 111 +7
==========================================
+ Hits 561 628 +67
Misses 182 182
Partials 3 3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| const result = await esbuild.build({ | ||
| entryPoints: [options.entrypoint], | ||
| platform: "neutral", | ||
| target: "deno1", // TODO: the versions should come from the user defined input |
There was a problem hiding this comment.
Esbuild requires a deno version for the target value, currently the user is not able to specify the version their app should run on. Once they are able to, I believe the version they specify should be used here
There was a problem hiding this comment.
Ran through the testing steps with the request-time-off app, works well! Something interesting I noticed re: the request-time-off app: the bundle size with Deno.bundle is 17kb and took 1.9 seconds to bundle. With esbuild, it ended up being 19kb and took 1.4 seconds.
I also tried it with my more involved / complex app interactive-approval app. esbuild increased the size by 2kb but the build time went from 1.1 seconds to 0.5 seconds!
This makes sense to me as I've heard esbuild is extremely fast. So, that's a nice thing here!
Very nicely done here, I left a few comments but you did an excellent job, especially with the tests. 💯
| output: () => Promise.resolve({ code: 0 }), | ||
| } as Deno.Command; | ||
|
|
||
| // Stub out call to `Deno.Command` and fake return a success |
| Deno.test("Deno Bundler tests", async (t) => { | ||
| await t.step(DenoBundler.bundle.name, async (tt) => { | ||
| await tt.step( | ||
| "should invoke 'deno bundle' successfully", |
There was a problem hiding this comment.
Technically we are not testing whether bundle is invoked here, we are testing whether Deno.Command was invoked. I think we may want to assert on the arguments passed into the Command stub to make sure it is bundle - assuming that's the right hypothesis we are testing.
There was a problem hiding this comment.
Makes sense, Ive added argument validation 💯
| Promise.resolve({ | ||
| code: 1, | ||
| stderr: new TextEncoder().encode( | ||
| "error: unrecognized subcommand 'bundle'", |
Summary
This PR aims to improve the behavior of the build hooks by adding a fallback to esbuild if
deno bundlefails.Testing
slack.jsonfileslack deploy -vvdeno bundlesrc/build.tsabove line 81 addthrow new BundleError()(this will force a fallback to esbuild)slack deployin your app folderFailed bundling with 'Deno Bundle' falling back to esbuildFeedback
Requirements