getsentry / sentry-javascript-bundler-plugins

JavaScript Bundler Plugins for Sentry
https://sentry.io
BSD 3-Clause "New" or "Revised" License
142 stars 37 forks source link

fix: Do not delete files before all upload tasks executed #572

Closed tyouzu1 closed 4 months ago

tyouzu1 commented 4 months ago

https://github.com/getsentry/sentry-javascript-bundler-plugins/issues/570

writeBundle object dont support vite v2, should solve real problems, the dependenciesAreFreedPromise has been used and will not wait a second time https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/c68361a636992e9f656f82f46fff031f1da18ba9/packages/bundler-plugin-core/src/plugins/sourcemap-deletion.ts#L35

In order to ensure that each deletion task will wait normally, it is necessary to recreate the dependenciesAreFreedPromise each time

tyouzu1 commented 4 months ago

@lforst I don't have enough time to test other frameworks(like webpack). But I think you should probably upgrade unplugin, I don't know if this has any impact, https://github.com/unjs/unplugin/issues/389

So I kept the original code logic, maybe it will look strange

const freeInitDependencyOnSourcemapFiles = createDependencyOnSourcemapFiles();
...
const freeDependencyOnSourcemapFiles = createDependencyOnSourcemapFiles();
...
freeInitDependencyOnSourcemapFiles();
freeDependencyOnSourcemapFiles();

Really sorry for the last change.

lforst commented 4 months ago

Thanks a bunch for the PR! Unfortunately, I don't understand how this PR will resolve the original problem. Would you mind adding a test case to highlight what exactly is fixed?

tyouzu1 commented 4 months ago

Thanks a bunch for the PR! Unfortunately, I don't understand how this PR will resolve the original problem. Would you mind adding a test case to highlight what exactly is fixed?

With the existing code, it seems that adding test cases requires a lot of effort. I don't dare to make too many changes, Or do you just need a demo?

lforst commented 4 months ago

@tyouzu1 if you're not comfortable adding tests, a demo would also be fine!

lforst commented 4 months ago

@tyouzu1 actually all good. Thanks to your explanation in the issue I grokked it now! Thank you!

tyouzu1 commented 4 months ago

@tyouzu1 actually all good. Thanks to your explanation in the issue I grokked it now! Thank you!

thank you for your patience

tyouzu1 commented 4 months ago

demo, the write hook will be called twice

vite.build({
  build: {
    sourcemap: true,
    rollupOptions: {
      output: {
        format: "cjs",
        entryFileNames: "[name].js",
      },
    },
  },
  plugins: [
    {
      name: "multi-write-bundle-test",
      enforce: "post",
      configResolved({ build: { rollupOptions } }) {
        rollupOptions.output = [
          {
            format: "system",
            entryFileNames: "[name]-plugin.js",
          },
          {
            format: "cjs",
            entryFileNames: "[name].js",
          },
        ];
      },
      generateBundle(options, bundle) {
        if (options.format === "system") {
          for (const name in bundle) {
            if (bundle[name]?.type === "asset" && !/.+\.map$/.test(name)) {
              delete bundle[name];
            }
          }
        }
      },
    },
    sentryVitePlugin({
      debug: true,
      org: "xx",
      project: "xx",
      authToken:
        "xxx",
      sourcemaps: {
        filesToDeleteAfterUpload: [`dist/**/*.js.map`],
      },
    }),
  ],
});
dist/index-plugin.js  142.33 kB │ gzip: 45.67 kB │ map: 347.64 kB
[sentry-vite-plugin] Info: Successfully uploaded source maps to Sentry
[sentry-vite-plugin] Debug: Deleting asset after upload: dist/index-plugin.js.map
[sentry-vite-plugin] Debug: Deleting asset after upload: dist/index.js.map
dist/index.html    0.37 kB │ gzip:  0.26 kB
dist/index.js    142.25 kB │ gzip: 45.63 kB │ map: 347.65 kB
[sentry-vite-plugin] Info: Successfully uploaded source maps to Sentry