feat(labs): add esbuild_bundle build rule #2405
Conversation
This is a wrapper around [esbuild](https://esbuild.github.io/).
BobobUnicorn
requested review from
alexeagle,
gregmagolan,
mattem and
mrmeku
as code owners
|
This is great! 🚀 Perhaps we can work on converging the two implementations from this PR and here? |
|
Oh, I didn't even realize there was one in progress! Yeah, I can try and merge them. Does this still belong in labs? |
|
I've rebased and greened up the original PR, and also merged some of what you've done here over. I think this should be fine outside labs. Did you find the addition of workers gave a big improvement here? I haven't experimented with workers for esbuild yet. I'm also in two minds about using the API over the CLI, ideally we could use the Go API to keep it native and fast, however that puts added dependencies on users for a Go toolchain and setup. Perhaps we can package that up nicely. I'll try and get the other PR landed, and we can iterate on the implementation there perhaps? |
| args.add_joined(["--platform", ctx.attr.platform], join_with = "=") | ||
| args.add_all(ctx.attr.loader, format_each = "--loader=%s") | ||
| args.add_all(ctx.attr.define, format_each = "--define=%s") | ||
| args.add_joined(["--target", "esnext"], join_with = "=") |
There was a problem hiding this comment.
Should the target be configurable? If not could you leave a comment saying why not?
| default = [], | ||
| ), | ||
| "tsconfig": attr.label( | ||
| doc = """The tsconfig file to use for the build. |
There was a problem hiding this comment.
Does esbuild require a tsconfig? I didn't think it did but I could be wrong. If its not required, I don't think the rule should make it required
| If you wish to use the default sourcemap behaviour, pass in "default".""", | ||
| default = "default", | ||
| ), | ||
| "splitting": attr.bool( |
There was a problem hiding this comment.
I think if you don't provide defaults to these boolean arguments they are considered required. Is that your intention? If not you might consider setting them to the esbuild defaults
| "devDependencies": { | ||
| "esbuild": "0.8.32", | ||
| "minimist": "^1.2.5", | ||
| "protobufjs": "6.8.8" |
There was a problem hiding this comment.
I believe these are all actual dependencies rather than dev dependencies.
| @@ -0,0 +1,59 @@ | |||
| load("//packages/jasmine:index.bzl", "jasmine_node_test") | |||
There was a problem hiding this comment.
This is a great initial test. If possible, making an integration test within the examples directory will test that the actual npm packaging logic is correct. Not required for labs, but it gives me some piece of mind when I make one
|
Hi there, did you coordinate at all with authors of #2252 ? I think that one is close to landing, so maybe we can refactor some of your work into there? |
|
We released 3.2.0 with If you'd like to sync or pair on adding new features, let me know! 😄 |
|
Yeah, sorry about disappearing; it's been a busy month for me. I'd be happy to add worker/plugin support, but esbuild is definitely plenty fast. |
This is a wrapper around esbuild.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Adds a new labs package.
Does this PR introduce a breaking change?
Other information