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

Proposal: Declarative Build Strategy with Mustache Template #16

Merged
merged 2 commits into from
Dec 20, 2024

Conversation

vnphanquang
Copy link
Contributor

@vnphanquang vnphanquang commented Dec 6, 2024

Description

While I was working on #12 and #14, I noticed that there was an opportunity to utilize a template-based build strategy instead of relying on string manipulation & concatenation as of now. This PR implements that. Please note that it changes the codebase significantly and I hope that doesn't come across as too ignorant. Happy to respect whatever decision you make as code owner.

As for benchmark, this new build strategy does not provide, nor aim at, any improvement over speed...

CPU | Intel(R) Core(TM) i5-4590 CPU @ 3.30GHz
Runtime | Deno 2.1.1 (x86_64-unknown-linux-gnu)

benchmark   time/iter (avg)        iter/s      (min … max)           p75      p99     p995
----------- ----------------------------- --------------------- --------------------------
old build           32.1 ms          31.1 ( 28.9 ms …  35.1 ms)  32.9 ms  35.1 ms  35.1 ms
new build           31.8 ms          31.5 ( 29.4 ms …  33.9 ms)  32.5 ms  33.9 ms  33.9 ms

summary
  old build
     1.01x slower than new build
View benchmark script
import * as path from 'jsr:@std/path';

Deno.bench('old build', { baseline: true }, async () => {
	await new Deno.Command('deno', {
		args: ['task', 'run', 'build'],
		cwd: path.resolve(import.meta.dirname ?? '', '../<path_to_git_worktree_at_main_branch>'),
	}).output();
});

Deno.bench('new build', { baseline: true }, async () => {
	await new Deno.Command('deno', {
		args: ['task', 'run', 'build'],
		cwd: path.resolve(import.meta.dirname ?? '', '../<path_to_git_worktree_at_pr_branch>'),
	}).output();
});

... but rather tries to make the codebase more maintainable down the line.

Quick Overview of Build Strategy

The build routine takes a list of configs, each essentially maps templates to corresponding build targets. All configs share the same interface.

template-based-build-strategy

@11bit
Copy link
Member

11bit commented Dec 16, 2024

In general, I like the idea of using templates. It should make it easier to add more build targets.

One thing, could you please move snapshots back to their folder - it makes things easier to review.

@vnphanquang
Copy link
Contributor Author

@11bit no problem. Do you prefer to co-locate the test file right next to source file as well? Although there are some tests that do not correspond to one specific source file so those will stay in the tests folder.

@11bit
Copy link
Member

11bit commented Dec 16, 2024

Yep, I prefer to colocate them. Thanks!

@vnphanquang
Copy link
Contributor Author

@11bit fixed via 6091caf. Let me know if that's good. Thanks a lot!

Copy link
Member

@11bit 11bit left a comment

Choose a reason for hiding this comment

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

Very nice. Thank you @vnphanquang !

@11bit 11bit merged commit d912338 into evilmartians:main Dec 20, 2024
1 check passed
@vnphanquang vnphanquang deleted the alter-build-strategy branch December 21, 2024 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants