Purescript bundle optimization issue.

[codygman]

If I'm understanding everything correctly, some transform that psc-bundle does eliminates a needed variable declaration. Minimum reproduction and some notes at the repo below:

https://github.com/codygman/purescript-optimize-issue

I also created an issue in purescript-contrib/pulp#227 since turning on optimization with pulp browserify ends up being affected by this runtime error bug due to a necessary var declaration being optimized away.

[paf31]

Is this running Browserify and then psc-bundle, or the other way round?

[codygman]

Browserify is first I think, but I'm not sure. If I'm wrong about this being a psc-bundle issue that means there are some transforms defined somewhere that pulp passes to browserify I think, however I couldn't find them.

EDIT: or a default browserify transformation could be eliminating this variable

[paf31]

I would say it's wrong to apply Browserify then psc-bundle, so maybe it is a Pulp issue then.

[hdgarrood]

pulp browserify -O does psc-bundle first, then browserify.

[megamaddu]

Also seeing this with pulp build, with or without -O. I just added a larger FFI file, but there are others that work fine and do similar things. Here's the top of the file that's breaking:

"use strict"

var keyPrefix = "Jane.Cache.Event:"
var localStorageTested = false
var localStorage = window.localStorage
var unprocessedEventQueue = []
var readyEventQueue = []
var completedEventQueue = []
var push = Array.prototype.push
var batchUnprocessedCount = 10
var batchUnprocessedInterval = 100
var batchReadyCount = 1000
var batchReadyInterval = 5000
var batchCompletedCount = 10
var batchCompletedInterval = 100
var postEvents = null

exports._enqueueEvent = function (event) {
  unprocessedEventQueue.push(event)
}

And here's that file in the bundle:

    "use strict";                       
  var unprocessedEventQueue = []
  var push = Array.prototype.push.apply
  var postEvents = null

  exports._enqueueEvent = function (event) {
    unprocessedEventQueue.push(event)
  }

It's consistent and only breaks on this file, which makes me think it has something to do with the contents of this file specifically..

[michaelficarra]

@spicydonuts Can you post the whole file? It's hard to tell whether a variable is used without being able to see all potential usages. Also, the line var push = Array.prototype.push.apply is concerning for a number of reasons.

1. That's not the line as listed in the input.
2. That would be an extremely odd way to retrieve Function.prototype.apply.
3. That would be an irresponsibly bad name choice for Function.prototype.apply.

[megamaddu]

@michaelficarra On push.apply, all correct, and once I got the code running I realized that and changed it haha.

I don't think there are any unused variables (and normally the linter would warn if that were the case), but there most definitely are used variables getting removed.

Here's the whole file:

"use strict"

var keyPrefix = "Jane.Cache.Event:"
var localStorageTested = false
var localStorage = window.localStorage
var unprocessedEventQueue = []
var readyEventQueue = []
var completedEventQueue = []
var push = Array.prototype.push
var batchUnprocessedCount = 10
var batchUnprocessedInterval = 100
var batchReadyCount = 1000
var batchReadyInterval = 120000
var batchCompletedCount = 10
var batchCompletedInterval = 100
var postEvents = null

exports._enqueueEvent = function (event) {
  unprocessedEventQueue.push(event)
}

function testLocalStorage () {
  if (localStorageTested) return
  localStorage.setItem("Jane.LocalStorageTest", "Success")
  localStorageTested = true
}

function handleUnprocessed (numberToProcess) {
  if (unprocessedEventQueue.length === 0) return
  var toProcess = unprocessedEventQueue.splice(0, numberToProcess)
  try {
    testLocalStorage()
    toProcess.forEach(function (event) {
      localStorage.setItem(
        keyPrefix + event.eventId,
        JSON.stringify(event)
      )
    })
    push.apply(readyEventQueue, toProcess)
  } catch (err) {
    console.warn("Failed to handle some unprocessed events", err)
    push.apply(unprocessedEventQueue, toProcess)
  }
}

function handleReady (numberToProcess) {
  if (readyEventQueue.length === 0) return
  var toProcess = readyEventQueue.splice(0, numberToProcess)
  postEvents(toProcess)(success, failure)

  function success () {
    push.apply(completedEventQueue, toProcess)
  }
  function failure (err) {
    console.warn("Failed to handle some ready events", err)
    push.apply(readyEventQueue, toProcess)
  }
}

function handleCompleted (numberToProcess) {
  if (completedEventQueue.length === 0) return
  var toProcess = completedEventQueue.splice(0, numberToProcess)
  try {
    testLocalStorage()
    toProcess.forEach(function (event) {
      localStorage.removeItem(keyPrefix + event.eventId)
    })
  } catch (err) {
    console.warn("Failed to handle some completed events", err)
    push.apply(completedEventQueue, toProcess)
  }
}

function restoreReady () {
  try {
    testLocalStorage()
    var keys = Object.keys(localStorage).filter(function (key) {
      return key.startsWith(keyPrefix)
    })
    var toRestore = keys.map(function (key) {
      return JSON.parse(localStorage.getItem(key))
    })
    push.apply(readyEventQueue, toRestore)
  } catch (err) {
    console.warn("Failed to restore ready events", err)
  }
}

exports._initializeEventQueues = function (postEventsFn) {
  postEvents = postEventsFn
  restoreReady()

  window.addEventListener("unload", function () {
    handleUnprocessed(unprocessedEventQueue.length)
  })

  setInterval(function () {
    handleUnprocessed(batchUnprocessedCount)
  }, batchUnprocessedInterval)

  setInterval(function () {
    handleReady(batchReadyCount)
  }, batchReadyInterval)

  setInterval(function () {
    handleCompleted(batchCompletedCount)
  }, batchCompletedInterval)
}

[megamaddu]

And here's that whole file in the bundle:

    "use strict";                       
  var unprocessedEventQueue = []
  var push = Array.prototype.push
  var batchUnprocessedCount = 10
  var batchUnprocessedInterval = 100
  var batchReadyCount = 1000
  var batchReadyInterval = 120000
  var batchCompletedCount = 10
  var batchCompletedInterval = 100
  var postEvents = null

  exports._enqueueEvent = function (event) {
    unprocessedEventQueue.push(event)
  }

  function testLocalStorage () {
    if (localStorageTested) return
    localStorage.setItem("Jane.LocalStorageTest", "Success")
    localStorageTested = true
  }

  function handleUnprocessed (numberToProcess) {
    if (unprocessedEventQueue.length === 0) return
    var toProcess = unprocessedEventQueue.splice(0, numberToProcess)
    try {
      testLocalStorage()
      toProcess.forEach(function (event) {
        localStorage.setItem(
          keyPrefix + event.eventId,
          JSON.stringify(event)
        )
      })
      push.apply(readyEventQueue, toProcess)
    } catch (err) {
      console.warn("Failed to handle some unprocessed events", err)
      push.apply(unprocessedEventQueue, toProcess)
    }
  }

  function handleReady (numberToProcess) {
    if (readyEventQueue.length === 0) return
    var toProcess = readyEventQueue.splice(0, numberToProcess)
    postEvents(toProcess)(success, failure)

    function success () {
      push.apply(completedEventQueue, toProcess)
    }
    function failure (err) {
      console.warn("Failed to handle some ready events", err)
      push.apply(readyEventQueue, toProcess)
    }
  }

  function handleCompleted (numberToProcess) {
    if (completedEventQueue.length === 0) return
    var toProcess = completedEventQueue.splice(0, numberToProcess)
    try {
      testLocalStorage()
      toProcess.forEach(function (event) {
        localStorage.removeItem(keyPrefix + event.eventId)
      })
    } catch (err) {
      console.warn("Failed to handle some completed events", err)
      push.apply(completedEventQueue, toProcess)
    }
  }

  function restoreReady () {
    try {
      testLocalStorage()
      var keys = Object.keys(localStorage).filter(function (key) {
        return key.startsWith(keyPrefix)
      })
      var toRestore = keys.map(function (key) {
        return JSON.parse(localStorage.getItem(key))
      })
      push.apply(readyEventQueue, toRestore)
    } catch (err) {
      console.warn("Failed to restore ready events", err)
    }
  }

  exports._initializeEventQueues = function (postEventsFn) {
    postEvents = postEventsFn
    restoreReady()

    window.addEventListener("unload", function () {
      handleUnprocessed(unprocessedEventQueue.length)
    })

    setInterval(function () {
      handleUnprocessed(batchUnprocessedCount)
    }, batchUnprocessedInterval)

    setInterval(function () {
      handleReady(batchReadyCount)
    }, batchReadyInterval)

    setInterval(function () {
      handleCompleted(batchCompletedCount)
    }, batchCompletedInterval)
  }

[megamaddu]

And sorry it took so long to reply; I missed your message somehow.

[megamaddu]

I ended up getting it to work by setting my two exports to null at the top of the file, then moving the rest of the file into a self-calling function.

[paf31]

So is this still considered a bug in psc-bundle then? psc-bundle should leave any JS alone which it doesn't recognize as coming from psc, so it certainly seems like it is.

[megamaddu]

I ended up rewriting this JS file last night but still saw the same issues (half of the vars removed). I also tried changing spaces for tabs and adding semicolons to every line. Same results, and the IIFE still fixes it. If it helps I can share the new version and its output.

[codygman]

@spicydonuts Please do share the new version and it's output.

[paf31]

Is it possible to reduce this down to a minimal test case please?

[menelaos]

Here is another way to reproduce this error:

• git clone https://github.com/purescript-contrib/purescript-argonaut-core
• cd purescript-argonaut-core
• bower install
• psc "src/**/*.purs" "bower_components/purescript-*/src/**/*.purs" "test/**/*.purs"
• psc-bundle "output/**/*.js" --module Test.Main --main Test.Main | node

This will result in a ReferenceError:

        var akeys = objKeys(a);
                    ^

ReferenceError: objKeys is not defined

[menelaos]

The following observation might be helpful:
The file "output/Data.Argonaut.Core/foreign.js" contains the following code:

function _compare(EQ, GT, LT, a, b) {
  // Body
}
exports._compare = _compare;

If that code is changed to

exports._compare = function _compare (EQ, GT, LT, a, b) {
  // Body
};

objKeys (which is used inside the body of the function) does not throw an error anymore.

[paf31]

Yes, it's important that foreign modules are structured in the right way so that psc-bundle can decide what to cut out and what to leave in.

[hdgarrood]

Ah, so we need to fix this in argonaut: https://github.com/purescript-contrib/purescript-argonaut-core/blob/576563b5fbf989d725b6f3128465c1ca45ecc0aa/src/Data/Argonaut/Core.js

I actually don't remember if we documented the requirements for psc-bundle anywhere, can anyone remind me?

Also, is it feasible to detect errors like this one at compile-time?

[hdgarrood]

This does indeed look like the kind of psc-bundle bug you thought it was, and I have a minimal reproducing case here: https://github.com/hdgarrood/psc-bundle-repro

psc-bundle seems to be leaving function declarations alone but removing statements like var x = y.

[codygman]

I suppose this hasn't been affecting others much or there would be more comments or duplicate issues. However, I feel like this is a pretty serious issue and should be resolved. I'll test later to see if it is still present with the latest version of Purescript.

[Pauan]

@codygman It affected me too, but I simply used a workaround (and nowadays I use rollup-plugin-purs), under the assumption that it would eventually be fixed.

I agree it should definitely be fixed if it's still present.