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

Polyfill is not defined when using spago bundle-app #34

Closed
ozkutuk opened this issue Aug 30, 2021 · 21 comments · Fixed by #36
Closed

Polyfill is not defined when using spago bundle-app #34

ozkutuk opened this issue Aug 30, 2021 · 21 comments · Fixed by #36
Labels

Comments

@ozkutuk
Copy link

ozkutuk commented Aug 30, 2021

Describe the bug
polyFill function is not exported from Typed.js, but called at the top-level of Typed.js itself. This seems to cause issues with bundling, where the definition isn't picked up by the bundle, causing the top-level polyFill() call to report Uncaught ReferenceError: polyFill is not defined.

To Reproduce
Initialize a spago project via spago init. Add the following list of dependencies to spago.dhall:

  [ "arraybuffer"
  , "arraybuffer-types"
  , "console"
  , "effect"
  , "prelude"
  , "psci-support"
  ]

Modify src/Main.purs to be as follows:

-- src/Main.purs
module Main where

import Prelude

import Data.ArrayBuffer.Types (ArrayView, Int32)
import Data.ArrayBuffer.Typed as ArrayBuffer
import Effect (Effect)
import Effect.Console (log)

main :: Effect Unit
main = do
  _ :: ArrayView Int32 <- ArrayBuffer.empty 0
  log "hello"

Bundle it and run it via node:

$ spago bundle-app
$ node index.js

Expected behavior
See the output: "hello".

Actual behavior

<path-to-project>\index.js:6
  polyFill();
  ^

ReferenceError: polyFill is not defined

Additional context
Running the bundle through browser or node does not make a difference, failing in both cases.

@ozkutuk ozkutuk added the bug label Aug 30, 2021
@JordanMartinez
Copy link
Contributor

Which version of purs are you using to produce this?

@jamesdbrock
Copy link
Member

jamesdbrock commented Aug 31, 2021

Thanks for the bug report @ozkutuk ! Do you have any suggestions about how this should be solved? Do you think we should export the polyFill function from Typed.js?

@jamesdbrock
Copy link
Member

@JordanMartinez
Copy link
Contributor

Reason why I ask is because of this issue: purescript/purescript#4044

@jamesdbrock
Copy link
Member

The PR which @JordanMartinez mentioned purescript/purescript#4044 was merged and included in purs 14.3. Jordan suspects that that PR might have fixed the bug which you’re seeing.

I just tried your bug reproduction steps with purs 14.3 and I got the expected output:

$ node index.js 
hello

purescript-arraybuffer wasn’t in package-sets for any purs 14.x versions prior to 14.3. What version of purs did you try this with @ozkutuk ?

@ozkutuk
Copy link
Author

ozkutuk commented Aug 31, 2021

Weird, purs --version reports 0.14.4. Additionally, spago version == 0.20.3 and package-set is psc-0.14.4-20210826. I don't think it has any relevance but I have tried this on a Windows machine and the node version is v14.13.1.

@ozkutuk
Copy link
Author

ozkutuk commented Aug 31, 2021

@jamesdbrock It seems that purescript/purescript#4044 didn't make it into 0.14.3 release, so I am curious how this works on 0.14.3 at all. Maybe this was a regression that is present in the 0.14.4 release? I may downgrade the purs to 0.14.3 and try again, and report the results.

@ozkutuk
Copy link
Author

ozkutuk commented Aug 31, 2021

I have bundled both with purs 0.14.3 and 0.14.4, and indeed it bundles and runs as expected on 0.14.3. To be sure that this is caused by purescript/purescript#4044, first I have compiled purs compiler on v0.14.4 tag myself and then got a bundle from that. Later, I have reverted the commit introducing purescript/purescript#4044 and rebuilt purs again, then getting another bundle output from the resulting purs executable. Indeed, the bundle of the latter purs executable contains the function polyFill() declaration, while the former does not.

I will try to further analyze why this is the case. I am not really familiar with the purescript codebase but the Bundle.hs seems pretty self-contained, so I may find out something. Now, it seems apparent that this not related to arraybuffer, so should we re-open this issue on the purescript repo instead?

Regarding the polyfill itself, caniuse.com reports that TypedArrays are supported since IE10, and the polyfill in question is not a full-fledged TypedArray polyfill either, so I think it would be a good idea to remove it anyway.

@jamesdbrock
Copy link
Member

Oh yeah you're right purescript/purescript#4044 was introduced in purs 14.4 https://github.com/purescript/purescript/blob/master/CHANGELOG.md#v0144

So your test

@JordanMartinez
Copy link
Contributor

Thanks all for the analysis!

@jamesdbrock
Copy link
Member

Here's another way to see this same bug.

In a clone of purescript-arraybuffer:

spago -x spago-test.dhall build
purs bundle --main Test.Main --module Test.Main --output index.Test.Main.js output/**/*.js
node index.Test.Main.js

@jamesdbrock
Copy link
Member

jamesdbrock commented Sep 8, 2021

Here's some archeology

5200641

f224bbf

#23 (comment)

@jamesdbrock
Copy link
Member

We should try to come up with a solution in this library which doesn't depend on waiting for a new purs version.

jamesdbrock added a commit that referenced this issue Sep 21, 2021
The `polyFill()` for TypedArray was causing problems with `purs bundle`
v0.14.4. #34

The link which the `polyFill()` function referred to is dead.

This link shows what appears to be a fully-functional TypedArray
polyfill, which is not what we had in our library. Was our `polyFill()`
even working at all? I don't know.

https://github.com/zloirock/core-js#ecmascript-typed-arrays
jamesdbrock added a commit that referenced this issue Sep 21, 2021
The `polyFill()` for TypedArray was causing problems with `purs bundle`
v0.14.4. #34

The link which the `polyFill()` function referred to is dead.

This link shows what appears to be a fully-functional TypedArray
polyfill, which is not what we had in our library. Was our `polyFill()`
even working at all? I don't know.

https://github.com/zloirock/core-js#ecmascript-typed-arrays
@JordanMartinez
Copy link
Contributor

@JordanMartinez
Copy link
Contributor

And here's all methods: https://caniuse.com/?search=typedarray

@JordanMartinez
Copy link
Contributor

As discussed in our call, we think a good way to handle this is to force users to handle the polyfill themselves. This is similar to what affjax does in requiring users to bring their own xhr2 for the library to work.

In other words, we would

  • update the readme to clarify that one must handle the polyfill themselves if they are working with older browsers
  • delete the polyfill
  • make a new release that is breaking

jamesdbrock added a commit that referenced this issue Sep 21, 2021
The `polyFill()` for TypedArray was causing problems with `purs bundle`
v0.14.4. #34

The reference link which the `polyFill()` function comments referred to is dead.

Add a section on polyfills to the README, with new reference links.
@jamesdbrock
Copy link
Member

Hi @ozkutuk I took your advice and just deleted the polyFill() function. I also published a new v11.0.2 with the change. Thanks for making this issue! Whatever happens with purs bundle now, I hope this solves your problem.

@JordanMartinez
Copy link
Contributor

@jamesdbrock Why wasn't this published as a breaking release (e.g. v12.0.0)?

@jamesdbrock
Copy link
Member

@jamesdbrock Why wasn't this published as a breaking release (e.g. v12.0.0)?

Yeah, it should be a breaking release.

@JordanMartinez
Copy link
Contributor

I believe fixing this would mean doing the following:

  • reverting the change
  • republishing a new v11.0.3, which is the same as v11.0.1
  • adding the change back in, except adding the readme update part, too
  • publishing a breaking release as v12.0.0

Ideally, we could avoid this work by just checking out v11.0.1 and publishing that tag as v11.0.3, but I'm not sure if that works or not.

@jamesdbrock
Copy link
Member

I believe fixing this would mean doing the following:

  • reverting the change
  • republishing a new v11.0.3, which is the same as v11.0.1
  • adding the change back in, except adding the readme update part, too
  • publishing a breaking release as v12.0.0

Ideally, we could avoid this work by just checking out v11.0.1 and publishing that tag as v11.0.3, but I'm not sure if that works or not.

Ok I'll do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 participants