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

Make Math.trunc polyfill optional? #16144

Closed
metagn opened this issue Nov 26, 2020 · 4 comments · Fixed by #19183
Closed

Make Math.trunc polyfill optional? #16144

metagn opened this issue Nov 26, 2020 · 4 comments · Fixed by #19183

Comments

@metagn
Copy link
Collaborator

metagn commented Nov 26, 2020

Summary

There is a Math.trunc polyfill that goes in every Nim JS file (except if -d:nodejs is passed) for IE support. It should be made opt-in through another define, or if it's possible, emitted by users themselves possibly through an imported module.

Description

Here is the polyfill:

Nim/lib/system/jssys.nim

Lines 781 to 792 in 3e7077a

# Workaround for IE, IE up to version 11 lacks 'Math.trunc'. We produce
# 'Math.trunc' for Nim's ``div`` and ``mod`` operators:
const jsMathTrunc = """
if (!Math.trunc) {
Math.trunc = function(v) {
v = +v;
if (!isFinite(v)) return v;
return (v - v % 1) || (v < 0 ? -0 : v === 0 ? v : 0);
};
}
"""
when not defined(nodejs): {.emit: jsMathTrunc .}

It's not that big of a deal by itself since it's only 156 extra bytes per JS output, and a bigger polyfill has already been removed, the 481 byte typed array polyfill removed in #16077, but I'm not proposing removal here as much as I'm proposing other options because Math.trunc is used way more often in Nim JS output and it would definitely slightly be disabling people who want their JS to work on IE <11.

My preferred solution would be some kind of iefills module (maybe another name), where Math.trunc and the typed number arrays would go. The user would import this, though you would have to make sure that iefills emits code before everything else, and I don't know the Nim constructs to do that (if there are any, #13376). This doesn't have to be in the standard library.

Another solution is instead of doing not defined(nodejs) you could do defined(ie) or a define with a longer and clearer name that you only define if you want IE/really old browser support.

Streamlining polyfills would make it easier for core Nim JS output to be more efficient. Bitsets currently work by setting fields in objects where the key is the value included in the set. 96.22% of users use browsers that support the Set type, there's no doubt Set and Map are faster compared to using object keys, but Nim can't use them. This might not be a perfect example because polyfilling Set in this case would be a bit cumbersome but it would be possible. We also don't need to delegate everything to JS builtins, the Nim standard library shares a lot with the C++ standard library but we still use the Nim versions. Polyfills also can't solve every problem with JS compatibility, they could end up making the resulting code perform way worse than before for older versions. My point is there are things that polyfills would help with.

Additional Information

  • Discussion originally spawned in PR that adds globalThis polyfill: globalThis shim #16054
  • Just doing -d:nodejs in cases where you want to use new browser features but aren't using node.js doesn't work great let alone make sense. The dom module defines dummy routines when nodejs is defined and wraps the real routines when it's not.
@Araq
Copy link
Member

Araq commented Nov 26, 2020

You would get much better JS output by making the JS codegen use injectdestructors.nim and replacing the NimCopy calls by the moves it instead produces. Nim's move semantics are very nice for the JS codegen too.

Seems much more useful than optimizing 156 bytes.

@metagn
Copy link
Collaborator Author

metagn commented Nov 26, 2020

I mentioned that it was 156 bytes specifically to point out that it wouldn't be much of an optimization to remove it, I made this issue primarily for the code generation to not have to rely on eldritch maneuvers rather than for the code to be necessarily faster or smaller. I think the JS code should directly reflect the Nim code (hence I want polyfills to be user code in Nim), implementing move semantics would really help in that regard but there are many of these issues, for example the JS cstring needing the extra mile to be supported in libraries, otherwise having to convert to string where if string was implemented the exact same way as cstring it would not make any difference to how the library would treat that object.

@Araq
Copy link
Member

Araq commented Nov 26, 2020

Surely we can do both -- just telling you where to get more juice.

@timotheecour
Copy link
Member

timotheecour commented Jan 12, 2021

there's no need to single out ie. Instead of std/iefills and defined(ie) I propose std/jspolyfills, see linked discussion timotheecour#509

rationale: each browser has their own limitations, for example BigUint64Array is supported in most browsers but not safari; a full fledged reusable module with API's like getBrowser, hasBigInt, as well as a way for user to specify on cmdline (hence at CT) which minimum browser he intends to support eg -—jsbrowser:chrome>=86,safari>=45; and maybe also a way to split polyfills into their own separate js file (see linked discussion).

EDIT

check whether something similar to useMagic can be used to conditionally generate the polyfill (ie, if trunc symbol isn't needed, the polyfill shouldn't even be generated, ie or not ie)

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

Successfully merging a pull request may close this issue.

4 participants