Skip to content

fix(esbuild): Don't use namespace for esbuild proxy resolving #311

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.

Sign up for GitHub

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

Merged
lforst merged 5 commits into from
Jun 12, 2023
Merged

fix(esbuild): Don't use namespace for esbuild proxy resolving #311

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
df26a12
fix(esbuild): Don't use namespace for esbuild proxy resolving
Jun 9, 2023
a076eba
Merge branch 'main' into fix-eslint
Jun 10, 2023
507cc75
Discard changes to packages/playground/build-esbuild.js
lforst Jun 10, 2023
5c24b30
Remove unnecessary namespace filter and definition
Jun 12, 2023
29f39f6
Remove unnecessary namespace filter
Jun 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions packages/esbuild-plugin/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,34 +38,42 @@ function esbuildReleaseInjectionPlugin(injectionCode: string): UnpluginOptions {

function esbuildDebugIdInjectionPlugin(): UnpluginOptions {
const pluginName = "sentry-esbuild-debug-id-injection-plugin";
const proxyNamespace = "sentry-debug-id-proxy";
const stubNamespace = "sentry-debug-id-stub";

return {
name: pluginName,

esbuild: {
setup({ onLoad, onResolve }) {
onResolve({ filter: /.*/ }, (args) => {
onResolve({ filter: /.*/, namespace: "file" }, (args) => {
Copy link
Copy Markdown
Member

@mydea mydea Jun 12, 2023

Choose a reason for hiding this comment

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

do we need this? We are just hard-coding file below 😅

Copy link
Copy Markdown
Author

@lforst lforst Jun 12, 2023

Choose a reason for hiding this comment

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

You are right, the namespace filter here is very likely unnecessary since we only want to resolve entrypoints which should only have the "file" namespace anyways (since "file" is the default).

The "passthrough" namespace below is then also unnecessary --> removed in 5c24b30

if (args.kind !== "entry-point") {
return;
}

return {
pluginName,
path: args.path,
namespace: proxyNamespace,
// needs to be an abs path, otherwise esbuild will complain
path: path.isAbsolute(args.path) ? args.path : path.join(args.resolveDir, args.path),
namespace: "file", // passthrough
pluginData: {
isProxyResolver: true,
originalPath: args.path,
originalResolveDir: args.resolveDir,
},
};
});

onLoad({ filter: /.*/, namespace: proxyNamespace }, (args) => {
onLoad({ filter: /.*/, namespace: "file" }, (args) => {
Copy link
Copy Markdown
Member

@mydea mydea Jun 12, 2023

Choose a reason for hiding this comment

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

also here, do we need namespace here?

Copy link
Copy Markdown
Author

@lforst lforst Jun 12, 2023

Choose a reason for hiding this comment

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

Yeah this namespace is also unnecessary. I think I defined it because I wanted to be conservative over what files we resolve/load, but thinking about it again, it seems unnecesary.

Thanks for questioning this!

Copy link
Copy Markdown
Author

@lforst lforst Jun 12, 2023

Choose a reason for hiding this comment

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

29f39f6

// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
if (!(args.pluginData.isProxyResolver as undefined | boolean)) {
return null;
}

// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
const originalPath = args.pluginData.originalPath as string;
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
const originalResolveDir = args.pluginData.originalResolveDir as string;

return {
loader: "js",
pluginName,
Expand Down