Skip to content
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.

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

Feature request: Allow to ignore uploading errors #31

Closed
ryo-utsunomiya opened this issue Mar 25, 2019 · 1 comment · Fixed by bugsnag/bugsnag-sourcemaps#38
Closed
Labels
scheduled Work is starting on this feature/bug

Comments

@ryo-utsunomiya
Copy link

ryo-utsunomiya commented Mar 25, 2019

Hi there,

When I run BugsnagSourceMapUploaderPlugin on Windows, sometimes I encountered ENOTEMPTY error.

[BugsnagSourceMapUploaderPlugin] uploading sourcemap for "https://www.ikyu.com/common/dist/chunks/31-df37450caf0ee52e3c02.js"
.\build-assets.bat : Error: ENOTEMPTY: directory not empty, rmdir 'C:\Users\appveyor\AppData\Local\Temp\1\bugsnag-sourcemapsm9vVAa'

This error occurs when removing not empty directories:
https://github.com/bugsnag/bugsnag-sourcemaps/blob/master/index.js#L345

The root cause of this error is Windows file system and there are no easy solutions.
isaacs/rimraf#72

In my opinion, failure of removing tmp directories is a minor issue. So, I want to ignore this error.

If this plugin provides some error handling option, I can ignore errors.

new BugsnagSourceMapUploaderPlugin {
  apiKey: '...',
  publicPath: '...',
  onError: (err, callback) => {
    if (err.code === 'ENOTEMPTY') {
      callback(); // ignore error
    } else {
      callback(err);
    }
  }
}

Modification may be something like this:

BugsnagSourceMapUploaderPlugin {
  constructor(options) {
...
+    this.onError = options.onError
  }
...
  apply(compiler) {
...
      parallel(sourceMaps.map(sm => cb => {
        console.log(`${LOG_PREFIX} uploading sourcemap for "${sm.url}"`)
        upload(this.getUploadOpts(sm), cb)
-      }), 10, callback)
+      }), 10, err => {
+        if (err && this.onError) {
+          this.onError(err, callback)
+        } else {
+         callback(err)
+        }
+      })
...
  }
}

What do you think?

@bengourley
Copy link
Contributor

The feature request sounds reasonable, but the implementation you suggested means that the occurrence of this error would still stop other source maps from being uploaded, it would just mean the plugin doesn't report the error.

I think the better thing to do would be for us to update the bugsnag-sourcemaps library (which is what this module delegates the upload task to) and tolerate ENOTEMPTY right where we attempt to remove it.

The change to this logic would look something like:

      fs.rmdir(options.tempDir, (err) => {
-       if (err) {
+       if (err && err.code !== 'ENOTEMPTY') {
          reject(err)
        } else {
          resolve()
        }

I'll schedule this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scheduled Work is starting on this feature/bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants