Always include CSS imports from live JS modules

[jgoz]

Closes #1370.

This change isn't exactly what you described in the linked issue (specifically traversing export * statements to find live .css files), but I think it represents the least surprising behavior when it comes to CSS imported from JS files:

With this change, CSS imported from JS will get included in output bundles if the CSS file itself is live and its direct importing JS file is also live. Previously, a dead ancestor would cause an otherwise live CSS file to be excluded.

This new behavior could potentially lead to unused CSS being included in bundles, but we can't determine that statically based only on the import graph. If and when CSS modules are supported, this behavior would remain valid for bare CSS imports, while CSS module imports could be analyzed for DCE based on their synthesized exports.

One thing to note, and a potential deviation from webpack's behavior, is that even with sideEffects: false, CSS imports will still be included if their parent JS is included. I believe this is because esbuild (correctly) considers all CSS files as having side effects since CSS modules are not currently supported. So even though sideEffects: false is not technically being respected, the resulting behavior is most likely what the user would be expecting.

[sqs]

I confirmed this PR fixes the issue for me on @sourcegraph's fairly large repo (and does not introduce any other issues as best I can tell). (We use export * from a lot for groups of UI components that themselves import CSS, so we're affected by this issue.)

I wonder, however, if a different approach to the fix would be better. This fix leaves the current markFileLiveForTreeShaking impl unchanged and special-cases the interpretation of its output (IsLive) for CSS. Instead, it might be better to change markFileLiveForTreeShaking to mark files with export * from as live. I.e., markFileLiveForTreeShaking should treat the following 2 files the same when calculating liveness: export * from 'foo' and export { a } from 'foo' (assuming that foo exports a). Currently it deems the former non-live and the latter live, which feels wrong to me.

(I defer to @evanw here, of course. And I'm happy to make a diff for the approach I proposed.)

[jgoz]

@sqs that's a great idea and it might avoid conflicts with any future CSS modules implementation, where CSS files may have non-live parts. I'll see if I can get it working.

[jgoz]

Did some investigation and now I remember why I didn't make the change in markFileLiveForTreeShaking. I think the problem is that parts containing side-effectful imports (in this case import './foo.css') don't track their dependencies within the source graph, unlike other kinds of imports. This means when marking a CSS import statement as being live, it's impossible to mark its parent import/export statements as also being live.

So I think the solution might be even earlier in the pipeline, possibly in scanImportsAndExports, though I'm not quite sure how to approach it.

[sqs]

I rebased this PR on the latest main branch and confirmed it still works on the large https://github.com/sourcegraph/sourcegraph esbuild project.

main...sqs:esbuild:fix-css-export-star-issue

I am not the PR author, so I can't update this PR, but I'm happy to make another PR for ease of merging.

@evanw: Would you consider merging this? Or do you have another preferred way of solving this? This would let us switch to esbuild at Sourcegraph, which would be a huge improvement because esbuild is awesome.

[jgoz]

@sqs I rebased this branch, but I'm not confident that this is how @evanw would want to approach the issue.

[sqs]

@jgoz Thank you!

@evanw Happy to pursue alternative approaches to this, if you would be willing to give some input. Thanks!