search

aspect-build / rules_esbuild

Bazel rules for https://esbuild.github.io/ JS bundler
https://docs.aspect.build/rules/aspect_rules_esbuild
Apache License 2.0
27 stars 27 forks source link

[Bug]: bazel_sandbox_plugin rejects some relative import paths #192

Open seh opened 7 months ago

seh commented 7 months ago

What happened?

Per preceding discussion in the "javascript" channel of the "Bazel" Slack workspace, when using the latest version of _rulesesbuild with the esbuild rule's new "bazel_sandbox_plugin" attribute (per #160) set to its default value of True, I find that TypeScript files that import other files from the same directory get rejected by the "bazel-sandbox" plugin. These files are referencing the adjacent files like this:

import classes from './Something.module.scss';

If I replace that with a bare file name, it seems to work without complaint:

import classes from 'Something.module.scss';

Is that intended behavior for this plugin? I see that it looks for paths that start with a period as one of its special cases that require it to carry on with remapping the resolved path under the execution root path.

It's possible for me to rewrite these paths in the import statements without the leading ./ as I showed above, but, unfortunately, when I do so, our linter programs can't process these files correctly. The only way I've found so far to allow our esbuild targets to continue building as they had been is to disable this plugin, setting the "bazel_sandbox_plugin" attribute to False.

Version

Development (host) and target OS/architectures: macOS, ARM64

Output of bazel --version: bazel 7.0.2

Version of the Aspect rules, or other relevant rules from your WORKSPACE.bazel or MODULE.bazel file:

Language(s) and/or frameworks involved: JavaScript, TypeScript

How to reproduce

No response

Any other information?

No response

Strum355 commented 3 months ago

Out of curiosity (a bit of a moonshot), do you think you could try the patch in this PR to see if it helps? Im wondering whether its the same symptoms + solution but for a different scenario

seh commented 3 months ago

I'm wondering whether its the same symptoms + solution but for a different scenario.

I haven't tried it yet, but I see that you're looking for paths that start with "..", but in our case, the paths start with only a single period, meaning "right here in this same containing directory".

Strum355 commented 3 months ago

I'm wondering whether its the same symptoms + solution but for a different scenario.

I haven't tried it yet, but I see that you're looking for paths that start with "..", but in our case, the paths start with only a single period, meaning "right here in this same containing directory".

Ah good point, what if you modify the following line to if (result.path.startsWith(".")) {? Anecdotally paths that are not absolute at this stage trip up the sandbox plugin, causing them to be rejected (even though they are valid within the sandbox, but its doesnt know that without the paths being made absolute), so that change should still solve my case and hopefully yours too