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

Possibly broken in IE11 #47

Closed
Miladiir opened this issue Sep 5, 2020 · 8 comments · Fixed by #48
Closed

Possibly broken in IE11 #47

Miladiir opened this issue Sep 5, 2020 · 8 comments · Fixed by #48

Comments

@Miladiir
Copy link

Miladiir commented Sep 5, 2020

I noticed that JSBI is broken in a test application when running in IE11.

When I exclude JSBI in webpacks babel-loader exclude (jsbi is parsed like the rest of my code):

grafik
This possibly points to:

if (!_(t) || i(t) !== t) throw new RangeError("The number " + t + " cannot be converted to BigInt because it is not an integer");

I use core-js@3 and regenerator-runtime as polyfills. Maybe I need something else as well?

When I do not exlude JSBI and treat it like any other dependency (jsbi is included in babel-loader):
grafik
This time it is unfortunately absolutely impossible to tell what upsets poor IE. Maybe its the same statement, who knows 🤷‍♂️.

Is the UMD version tested? Works fine in Firefox 80, but not in IE11.

I also noticed #42. I tested importing the common-js and mjs versions directly, however I still get similar results. IE is broken while Firefox works.

@glr0221
Copy link

glr0221 commented Sep 7, 2020

@Miladiir

I was about to use this library for a project of mine. Did this library ever work for IE11? I mean if there was a version of this library that worked for IE11? Currently i need support for two core objects/functions BigInt() and getBigUint64(). Thank you.

@Miladiir
Copy link
Author

Miladiir commented Sep 7, 2020

@Miladiir

I was about to use this library for a project of mine. Did this library ever work for IE11? I mean if there was a version of this library that worked for IE11? Currently i need support for two core objects/functions BigInt() and getBigUint64(). Thank you.

I do not know whether it ever worked. However currently definetly not. https://github.com/GoogleChromeLabs/jsbi/blob/5e757a2d1c51e3722fe7c59edbecf1be93ff187e/.babelrc which is the version on master as of today does not include IE11 as a target. The issue is related to the fact, that I cannot even get to get it working, even if I set the target to include IE11 myself. Maybe I can get it working. In that case I will ping you.

@jakobkummerow
Copy link
Collaborator

jakobkummerow commented Sep 7, 2020

Looks like IE11 doesn't support Math.imul and Math.clz32, which JSBI uses. At least, that's the result I get when I comment out "minify()" in rollup.config.js, regenerate the UMD version, and test var a = JSBI.BigInt('12345678901234567890') with that on an IE11 VM.

As a short-term workaround, you can provide a very simple polyfill (which does not have the proper semantics, but is good enough for JSBI):

if (!Math.imul) {
  Math.imul = function(a, b) {
    return ((a & 0xFFFF) * (b & 0xFFFF)) | 0;
  }
}
if (!Math.clz32) {
  Math.clz32 = function(x) {
    if (x === 0) return 32;
    return 31 - (Math.log(x >>> 0) / Math.LN2 | 0) | 0;
  }
}

If you're worried about breaking other modules that might want to use these functions, you can find functionally fully correct polyfills on MDN.

I know next to nothing about Babel. Presumably it has a way to polyfill Math functions, but I don't know how to make it do that.

@Yaffle
Copy link
Contributor

Yaffle commented Sep 7, 2020

@jakobkummerow ,

if (!Math.clz32) {
  Math.clz32 = function(x) {
    if (x === 0) return 32;
    return 31 - Math.floor(Math.log((x >>> 0) + 0.5) * Math.LOG2E);
  }
}

@jakobkummerow
Copy link
Collaborator

@Yaffle : interesting fix, thanks! I actually copied that line from MDN; turns out different translations of the Math.clz32 page provide different polyfills, at least two of them omit the +0.5 (which I can confirm is buggy e.g. for x = 0x10000000), a few others use / Math.LN2 instead of * Math.LOG2E, which also avoids the issue (yay for finite precision math!). Oh well, or one could implement bit fiddling.

I'll edit my earlier post to avoid any confusion.

@Miladiir
Copy link
Author

Miladiir commented Sep 9, 2020

Does that mean that the target only has transformed syntax, but does not include the proper polyfills? Thats really strange. Is that a misconfiguration or on purpose. Maybe a maintainer can clarify. I thank you for figuring out the problem.

One thing I'd like to add though, is that polyfills on MDN are not alway according to standard (or correct at all). For proper polyfills for ECMA standard behaviour, please take a look at core-js. One does not need to import the whole project, but it is quite modular.

As I wrote in the original post:

I use core-js@3 and regenerator-runtime as polyfills. Maybe I need something else as well?

Which I still find really strange. For some reason my configuration did not pick up on the missing functionality in my target, or the required shims are missing in my libs. I will try to find the issue sometime this week hopefully. I'll keep you updated.

@mathiasbynens
Copy link
Member

Looks like IE11 doesn't support Math.imul and Math.clz32, which JSBI uses. At least, that's the result I get when I comment out "minify()" in rollup.config.js, regenerate the UMD version, and test var a = JSBI.BigInt('12345678901234567890') with that on an IE11 VM.

As a short-term workaround, you can provide a very simple polyfill (which does not have the proper semantics, but is good enough for JSBI):

if (!Math.imul) {
  Math.imul = function(a, b) {
    return ((a & 0xFFFF) * (b & 0xFFFF)) | 0;
  }
}
if (!Math.clz32) {
  Math.clz32 = function(x) {
    if (x === 0) return 32;
    return 31 - (Math.log(x >>> 0) / Math.LN2 | 0) | 0;
  }
}

If you're worried about breaking other modules that might want to use these functions, you can find functionally fully correct polyfills on MDN.

IMHO this would be the best approach, except preferably we wouldn’t install on Math, but just keep these as local helper utilities that we don‘t expose globally or on any built-ins.

jakobkummerow added a commit that referenced this issue Sep 9, 2020
Fall back to custom implementations otherwise.
This is for IE11 compatibility.

Fixes #47.
mathiasbynens pushed a commit that referenced this issue Sep 9, 2020
Fall back to custom implementations otherwise.
This is for IE11 compatibility.

Fixes #47.
@glr0221
Copy link

glr0221 commented Sep 10, 2020

I just wanted to say, I have successfully used JSBI with IE. Thank you so much @jakobkummerow @mathiasbynens @Miladiir

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants