alexei / sprintf.js

sprintf.js is a complete open source JavaScript sprintf implementation
BSD 3-Clause "New" or "Revised" License
2.11k stars 291 forks source link

What browsers does sprintf.js targets? #137

Closed nazar-pc closed 7 years ago

nazar-pc commented 7 years ago

What I'm actually curious about is whether you're fine with dropping IE entirely and only support last 2 versions of any evergreen browser in next major release? If this is the case it would be possible to reduce and simplify code a bit.

alexei commented 7 years ago

I haven't had access to any IE for a couple of years now so I don't really know what [still] works and what doesn't. Afaik IE > Edge so dropping it entirely is not really an option. Do note however the cache is based on a pure object so that means IE >= 11 I think. So then what optimizations are you thinking about? i.e. that would still include IE 11

ArmorDarks commented 7 years ago

According to our metrics, dropping IE definitely not an option so far, unfortunately.

nazar-pc commented 7 years ago

For instance, all modern browsers (IE is not modern) support String.prototype.repeat(), so str_repeat() is not needed and native implementation should be faster (haven't tested yet). This was the first one I've noticed.

@ArmorDarks, I do not suggest deleting all of existing versions from Bower or NPM, you can use older version that supports IE as long as you need to. I just don't think that those who work with modern browsers should pull polyfills for something as simple as String.prototype.repeat() if their browsers have it built-in faster implementation. Also you can add String.prototype.repeat() polyfill for your project separately and use newer versions of this library, it just wouldn't be bundled anymore.

Don't you agree it is always easier to add stuff you need rather than remove something you don't need?

alexei commented 7 years ago

Indeed one can use a polyfill for String.prototype.repeat() so I agree to using it.

I looked Object.create up on the Internet and noticed there are polyfills for it too. Though I'm not so sure they allow calling it with null as a parameter.

alexei commented 7 years ago

@nazar-pc if you have suggestions likes this one, please open an issue for each one and mark them as enhancement or something. Or drop the list here and I'll create them myself.

nazar-pc commented 7 years ago

I'll just look around for improvements and will create a PR with the list of changes, so we can discuss them concretely there. Will close this issue for now.

ArmorDarks commented 7 years ago

Just don't forget to document all things, that should be polyfilled for IE. I imagine that digging into internals to find out which ones should will be quite not enjoyable ride...

nazar-pc commented 7 years ago

Sure

idubinskiy commented 7 years ago

@alexei FYI, version 1.1.1's use of String.prototype.repeat broke one of our apps that was still using Node 0.12.

nazar-pc commented 7 years ago

Node 0.12 is EOL for almost half a year now, you should really consider upgrade for security and performance reasons. If that is impossible - just include String.prototype.repeat polyfill and it should work just fine.

idubinskiy commented 7 years ago

@nazar-pc We're actually in the process of upgrading our apps, including that one. However, it's still unpleasant to have a patch release include a change that broke the next build of our application.

nazar-pc commented 7 years ago

Well, I think it would be useful to add a compatibility table for supported browsers and node.js versions. If that is dynamic like latest - 1 then you'll be able to set a fixed version in dependencies or if fixed versions are specified then you can safely rely on semver, but at least it would be great to have that specified explicitly like this (example from https://github.com/webcomponents/webcomponentsjs):

Polyfill IE11+ Chrome* Firefox* Safari 9+* Chrome Android* Mobile Safari*
Custom Elements
HTML Imports
Shady CSS/DOM

*Indicates the current version of the browser

ArmorDarks commented 7 years ago

Well, @idubinskiy is right that such breaking change shouldn't be released as patch....

alexei commented 7 years ago

@idubinskiy if it broke on your live server, well good for you!

@ArmorDarks I thought semver refers to public API, not implementation details 😕

Re compatibility table: what do you guys use to test JS in various browsers? I mean how would one generate a compatibility table automatically?

ArmorDarks commented 7 years ago

@alexei It isn't just implementation detail. It's breaking change, which broke lib in old Node and IE9, thus compromised API for those consumers. I don't care much, because I nor use Node 0.12, nor target IE9, but I'm quite sure on some people it came unexpectedly, especially considering that it was bumped with patch, thus passed ^ selector.

| don't want to sound rude. Just want to eliminate misconception about semver.

Well, now there is not much can be done about it. You still can drop old version or revert changes and make proper releases chain, but I think most people already done something with it anyway.

idubinskiy commented 7 years ago

This is a somewhat relevant discussion on the SemVer repo: https://github.com/mojombo/semver/issues/311

It's certainly not a settled matter. Just wanted to point that perhaps more care should have been taken. As far as I can tell, the README wasn't updated even with the known incompatibility for older browsers requiring a polyfill.

alexei commented 7 years ago

I updated the readme file and I apologize for not doing so earlier.

alexei commented 7 years ago

Also I opened an issue to remind myself to set up some automated browsers tests. Any pointer would be appreciated.