feat(esbuild): add output_css flag to esbuild() #2545
Conversation
dae
requested review from
alexeagle,
gregmagolan,
mattem and
soldair
as code owners
|
Are there other outputs esbuild will generate? CSS maps for example, or others unrelated to css. |
|
(force-pushed to remove the two unnecessary link_workspace_root lines) A brief scan of the docs seem to indicate it's just the .css file for now, and --summary does not show other unexpected files being created. I'm new to esbuild though, so it's possible there's some other feature I missed. They have an issue about sourcemap support for css: evanw/esbuild#519 |
|
I don't think we can infer the outputs, because esbuild changes its behaviour depending on whether imports are found in the files it processes, so we'd need to parse the files ourselves to know what it's going to do. additional_outputs would work, with the minor downside that it's inconsistent - the caller gets the ".js" appended to the name for free, but is responsible for appending ".css" itself |
|
I've rebased on latest main and added a fix for esbuild 0.11, and buildkite's passing. Not sure why circleci is complaining - we had a similar issue in a previous PR: #2564. |
packages/esbuild/esbuild.bzl
Outdated
| @@ -273,14 +283,16 @@ For further information about esbuild, see https://esbuild.github.io/ | |||
| """, | |||
| ) | |||
|
|
|||
| def esbuild_macro(name, output_dir = False, **kwargs): | |||
| def esbuild_macro(name, output_dir = False, output_css = False, **kwargs): | |||
There was a problem hiding this comment.
Now after the rebase, what happens if the user passes output = "foo.js" when the rule is named bar? I'm guessing esbuild will output foo.css and bazel wont find the output?
I guess now we have the same issue with output_map // cc @mattsoulanille
There was a problem hiding this comment.
Sorry, didn't realise that had been changed. Can we rely on the user providing a filename with an extension that I can use split_extension() on?
There was a problem hiding this comment.
I don't think we can rely on the output file being split_extension()-able. I'm pretty sure the user is allowed to provide a file that has no extension at all, but I haven't tested it.
@mattem I think we need to remove the output_map arg. Esbuild (as far as I can tell) doesn't allow you to specify where the sourcemap gets created and always bases the path off of the output filename. The rule implementation also only uses output_map to tell bazel about the sourcemap and doesn't even pass it to esbuild. We should probably compute the path in the rule implementation instead of passing it as an arg.
There was a problem hiding this comment.
dae@dip:~% esbuild --bundle foo.js --outfile=bar.js
bar.css 24b
bar.js 15b
⚡ Done in 2ms
dae@dip:~% esbuild --bundle foo.js --outfile=bar
bar.css 24b
bar 15b
⚡ Done in 2ms
So it looks like stripping the extension could work. I have no preference either way, so please just let me know whether you'd prefer the current boolean behaviour be fixed, or it changed to accept a filename directly.
There was a problem hiding this comment.
Ah so looks like esbuild will always correctly identify the filename and append .css, so if there is an extension we can strip it and use it for the output_css name, rather than a boolean. It would be good if the two properties in the API had the same semantics.
@mattsoulanille True, however it also serves as a label that can be then addressed directly, eg a user could have //foo:bar.js.map as an input to a downstream rule. This comes in handy when you need to route both the .js and .js.map somewhere, but the .css to another rule rather than depending on the full //foo:bundle target.
What we should do though is remove the .metadata and .jsconfig from the default output 😅
There was a problem hiding this comment.
Sorry, I'm a bit confused - you talk about stripping, but also removing the boolean, whereas I would have thought stripping is only useful in the boolean case. Which would you prefer? :-)
There was a problem hiding this comment.
Oops sorry. Upon thinking about it a bit more, perhaps we don't do anything in the macro at all, and leave it completely up to the user to provide the correct name via output_css from the rule, and pass it with everything else via **kwargs. This way, if esbuild changes to allow control over the resulting css filename there's no change to our API, we would just have to pass an additional flag with the existing output_css name that we get, and the user can simply customize it by changing this existing attribute. wdyt? Sorry, I know we've gone around the houses here!
There was a problem hiding this comment.
No problem, done. If you'd like all the changes squashed into a single commit, please let me know.
| "--resolve-extensions=.mjs,.js", | ||
| ], | ||
| entry_point = "main.ts", | ||
| output_css = True, |
There was a problem hiding this comment.
This should be main.css now right?
There was a problem hiding this comment.
Sorry, made the change in haste. Have pushed a fix.
When esbuild encounters an 'import "./foo.css"' file in normal (non-splitting) mode, it will place the contents of that file in a .css file next to the main output .js file. This patch adds a flag output_css, which when set to True, will declare the created .css file as another output, so it can be consumed by other rules.
|
Thanks! 🚀 |
When esbuild encounters an 'import "./foo.css"' reference in normal (non-splitting)
mode, it will place the contents of that file in a .css file next to the
main output .js file.
This patch adds a flag output_css, which when set to True, will declare
the created .css file as another output, so it can be consumed by other
rules.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?