Donaldcwl / browser-image-compression

Image compression in web browser
MIT License
1.31k stars 161 forks source link

fix: #17 stop variable shadowing #33

Closed tomsaleeba closed 4 years ago

tomsaleeba commented 4 years ago

There's a bit of noise in this commit with all the coverage stuff being in version control but the main change is to web-worker.js.

Background: I ran without the minified/built version of the library to get some more info on the problem and got the following stack trace:

Run compression in web worker failed: Error: lib is not defined
ReferenceError: lib is not defined
    at blob:https://example.com/4c9447b0-8a31-4f47-ba11-d7f326679d7a:12
    at Worker.handler (web-worker.js:79)
    at Worker.sentryWrapped (helpers.js:74) , fall back to main thread

...and this relates to this code that is running in the web worker:

(() => {
  let scriptImported = false
  self.addEventListener('message', async (e) => {
    const { file, id, imageCompressionLibUrl, options } = e.data
    try {
      if (!scriptImported) {
        // console.log('[worker] importScripts', imageCompressionLibUrl)
        importScripts(imageCompressionLibUrl)
        scriptImported = true
      }
      // console.log('[worker] self', self)
      const compressedFile = await lib(file, options) // stack trace relates to this line!!!
      self.postMessage({ file: compressedFile, id })
    } catch (e) {
      // console.error('[worker] error', e)
      self.postMessage({ error: e.message + '\n' + e.stack, id })
    }
  })
})()

So after looking at web-worker.js I could see some variable shadowing. The ES module import at the top of the file and the function that comes into scope from the importScripts() call both have the same name. This doesn't seem to cause an issue in your example but I'm assuming this confuses bundlers, like webpack which I'm using.

Changing one of the variable names fixed the issue.

tomsaleeba commented 4 years ago

I've just had a go a breaking that cyclic dependecy.

tomsaleeba commented 4 years ago

This PR still doesn't fix the fact that this code is not minify/uglify friendly. The following patch shows a proof-of-concept for a horribly hacky workaround that might work but it won't be nice to maintain.

First, we need to stop the call of compress from being touched so we use a string for the function name. Then, for all the helper functions from util, we need to try getting it from the global scope (for when we're a web worker) or just calling it (for when we're processing on the main thread). I've only changed one call here, and it works. If we pursue this, all other calls will need to be changed too.

Right now, I'm getting this error (highlighting the next function call to be updated) when this patch is applied:

Run compression in web worker failed: Error: d is not defined
ReferenceError: d is not defined
    at a (b99a9c7e-f193-496a-9de1-f5568e95afe9:3)
    at b99a9c7e-f193-496a-9de1-f5568e95afe9:3
    at Worker.e (browser-image-compression.mjs:8)
    at Worker.r (helpers.js:74) , fall back to main thread
// snip...
drawFileInCanvas = function l(e) {
    return new Promise((function(t, n) {
        var r, i, a = function() {
            try {
                return i = d(r), // <-- error on this line
                t([r, i])
            } catch (e) {
                return n(e)
            }
// snip...

Here's the patch:

diff --git a/lib/image-compression.js b/lib/image-compression.js
index 30b2533..7e1c4ec 100644
--- a/lib/image-compression.js
+++ b/lib/image-compression.js
@@ -20,6 +20,7 @@ import {
  * @returns {Promise<File | Blob>}
  */
 export default async function compress (file, options) {
+  const self = this.self || {}
   options.maxSizeMB = options.maxSizeMB || Number.POSITIVE_INFINITY

   if (!(file instanceof Blob || file instanceof CustomFile)) {
@@ -33,7 +34,7 @@ export default async function compress (file, options) {
   const maxSizeByte = options.maxSizeMB * 1024 * 1024

   // drawFileInCanvas
-  let [img, canvas] = await drawFileInCanvas(file)
+  let [img, canvas] = await (self.drawFileInCanvas || drawFileInCanvas)(file)

   // handleMaxWidthOrHeight
   canvas = handleMaxWidthOrHeight(canvas, options)
diff --git a/lib/web-worker.js b/lib/web-worker.js
index ada7596..de09df2 100644
--- a/lib/web-worker.js
+++ b/lib/web-worker.js
@@ -30,7 +30,12 @@ const worker = createWorker(() => {
         scriptImported = true
       }
       // console.log('[worker] self', self)
-      const compressedFile = await compress(file, options)
+      const globalWorkerScope = self
+      // we need to address the target function using a string because
+      // minifiers/uglifiers will break the linkage otherwise as they cannot see the
+      // function we define and import with `importScripts()`. Find the other
+      // comment with "MUSTMATCH" to see where this links to.
+      const compressedFile = await globalWorkerScope['compress'](file, options)
       self.postMessage({ file: compressedFile, id })
     } catch (e) {
       // console.error('[worker] error', e)
@@ -63,7 +68,7 @@ export function compressOnWebWorker (file, options) {
     CustomFile = File

     function _slicedToArray(arr, n) { return arr }
-
+    // MUSTMATCH: this function name must match what we call above
     function compress (){return (${compress}).apply(null, arguments)}
     `)
     }
Donaldcwl commented 4 years ago

Thank you for spotting this issue, I have refactored the code in Web Worker to address it.

Pardeep94 commented 4 years ago

I still facing the same issue.