Remove partially shimmed support.

[jdalton]

Shims that cover only part of the primary function of an ES5 method are potentially dangerous because they provide a false sense of support.

Devs checking with truthy inferences may assume for example that if (Object.create) { ... } it is fully functional Object.create(null). We should remove all shims that cover only part of a methods functionality.

[jdalton]

Off the top of my head that means removing

Object.create()
Object.defineProperty()
Object.defineProperties()

To continue the theme of helping devs I think we should delete them or wrap existing methods that are buggy, for example buggy IE Object.defineProperty(), and throw appropriate errors.

[robotlolita]

@jdalton
I think that, from a practical standpoint, these partially shimmed are interesting to the end-user. Bear in mind that es5-shim is geared towards end-users, the ones that control the environment they're using the library on. Library authors shouldn't be relying on the presence of such shims, they should decide upfront if they're going to support or not legacy engines, and tell their users what they expect to be provided by the platform for the library to run.

The current philosophy of es5-shim is:

"As closely as possible to ES5" is not very close. Many of these shims are intended only to allow code to be written to ES5 without causing run-time errors in older engines. In many cases, this means that these shims cause many ES5 methods to silently fail. Decide carefully whether this is what you want.

The README tells the user that up-front, highlighting all the problems with the provided shims, which I do think is the right thing to do.

[jdalton]

Partially working shims only cause confusion.
Libs that fork for ES5 additions most commonly use truthy inferences and so this will only lead to problems w/ partially implemented shims.

Take a look at any JS lib, (jQuery, Underscore.js, MooTools, and the like), they all assume the presence of the method means it is completely functional (maybe a little shortsighted but that's the way it is).

"I want ES5 methods to silently fail" — no dev ever

[aFarkas]

I really think jdalton is right. I'm using ES5 in my scripts and I always delete those + some others. And I think it is really bad practice to add methods, which do not work.

Additionally, I don't think, that the scripts, which only do test presence, are shortsited. If they would test more, they would only test for known browser bugs or known partial implementation by browsers. But it's a little bit an overkill to test, wether it's a "stupid" self-made implementation.

If you want to have partial implementation with the same API in all browsers add a new global object with those methods:

es5shim.objectCreate = (noCreateSupport) ? 
    function(){
          //partial implementation
    } :
    Object.create
;

[Yaffle]

Do you remember this problem?

var x = [1, 2, 3];
for(var i in x) {
// without any hasOwnProperty filterring...
}

I think, that it's better to

1. remove Object.create/define..... except Object.keys

And sometimes it's better to replace such things:
if (!Array.prototype.indexOf) {
Array.prototype.indexOf = ..
with this:
Array.prototype.indexOf = ...
to make all browsers works identically

[Raynos]

Don't remove Object.create / Object.defineProperty / Object.defineProperties.

It will just be frustrating to those of us who know how to use the safe subset of those functions.

As for devs assuming object.create works if it exists consider the following two situations

1. Library sees Object.create doesn't exist but wants to use ES5 only features. He then throws an error because he can't possibly use these features
2. Library sees Object.create does exist, he wants to use ES5 only features. He tries to use ES5 features, ES5-shim throws an error because he cant possibly use those features

Nothing's changed, in both cases the error get's thrown.

The only thing this change will do is create a fork in the ES5-shim where half us want partial implementations and the other half don't. That will be annoying as hell.

Now if you have a problem with ES5 implementations silently failing, then fix that. Don't remove partial implementations.

[jdalton]

@Raynos

It will just be frustrating to those of us who know how to use the safe subset of those functions.

As for devs assuming object.create works if it exists consider the following two situations

1. Library sees Object.create doesn't exist but wants to use ES5 only features. He then throws an error because he can't possibly use these features

2. Library sees Object.create does exist, he wants to use ES5 only features. He tries to use ES5 features, ES5-shim throws an error because he cant possibly use those features

Naw, if it exists it should work as expected.
There should be no need to only use a subset of the functionality.

You missed
3) Devs checks if a feature exists like Object.create() else falls back to a less desirable and maybe not as robust code fork.

Devs should be allowed to decide how to handle their own feature forks, implementing a broken shim is pointless.

[Raynos]

@jdalton Implementing a limited shim is far from pointless

I quite like being able to use object.create on all non-null values (for the first parameter) and for limited property descriptors (i.e. no getter/setter).

What code should be doing is not checking for Object.create but check whether Object.create(null).toString is a value.

We should not change the ES5-shim to work with other people's bad code.

Besides I have yet to see a solid real use case where having Object.create / Object.defineProperty in the ES5 shim is a bad thing. All I've heard is speculation and maybes

[jdalton]

@Raynos

I quite like being able to use object.create on all non-null values (for the first parameter) and for limited property descriptors (i.e. no getter/setter).

Sounds like a perfect thing to do in your own code forks/fallbacks but not on smth exposed on the public API.

We should not change the ES5-shim to work with other people's bad code.

If you expose methods to global Objects that are clearly spec'ed it's probably not a good idea to throw un-expected behavior into the mix. Good code, bad code, all popular libraries and frameworks will get tripped up.

Besides I have yet to see a solid real use case where having Object.create / Object.defineProperty in the ES5 shim is a bad thing. All I've heard is speculation and maybe

I've seen bad shims in Prototype.js and other libs cause problems time and time again.

Having a shim is not a bad thing if you can reproduce functionality. My own work related projects make heavy use of Object.create(null) and descriptors and having a half-working shim is of little use.

[Raynos]

have seen bad shims in Prototype.js and other libs cause problems time and time again.

I see your argument now, I see the light.

I'd have no objection, if the "incomplete" shims were not injected into global scope by default but we exposed an es5shim.incompleteShims() method instead. (something similar like pd.extendNatives

I would however find it rather annoying if this code was removed from the main es5-shim branch completely.

[domenic]

I think this issue is more complicated than it appears. The ES5 spec is involved and not entirely trivial to follow to the letter, and demanding that we either do so or hide behind an opt-in mechanism brings up a lot of concerns:

• Function.prototype.bind would be removed---despite working for the vast majority of use cases---due to the caveats listed in the readme. Interestingly I believe this is the only shimmed method in current JavaScriptCore.
• In engines supporting __proto__ and/or __(define|lookup)(Getter|Setter)__, much of the behavior of Object.create, Object.getPrototypeOf, Object.getOwnPropertyDescriptor, etc. work well currently, even if enumerable/writable/configurable cannot be set correctly. This would be lost.
• Object.freeze, Object.seal, and Object.preventExtensions have great applicability in secure JavaScript work like Mark Miller's. But for most of us, they're used to prevent mistakes during development or to send a message to the library consumer. Having them silently do nothing on legacy engines is not harmful since they are mainly development aids. Should we still cut them out?
• Even modern engines don't follow many aspects of the spec correctly, as evidenced by test-262 failures. Is it fair to demand es5-shim be better than they are? Is it helpful for es5-shim to be incompatible with them? Should es5-shim test for spec violations and try to replace the implementation where possible? Should es5-shim test for spec violations and remove the native implementation if it does not conform? I believe this has come up several times already with regard to date parsing, actually.

As an example of the last, somewhat-recent versions of JavaScriptCore (possibly current ones too) give Object.create(null).hasOwnProperty("__proto__") === true. If, as this issue suggests, es5-shim's goal is to bring all engines in line with the ES5 spec or die trying, then it should remove Object.create from JavaScriptCore.

I would think that removing unsafe shims (and probably being more rigorous about determining which those are, perhaps by running them through the test-262 suite) is the job of a fork.

[jdalton]

@DomenicDenicola

Function.prototype.bind would be removed---despite working for the vast majority of use cases---due to the caveats listed in the readme. Interestingly I believe this is the only shimmed method in current JavaScriptCore.

Not so. I am asking to remove shims that can't match functionality.
Discrepancies between a method's length property, or if smth has a prototype property, or other edge-case concerns are less important (especially with Function#bind because its popularized form in PrototypeJS lacked a lot of these details).

In engines supporting proto and/or (define|lookup)(Getter|Setter), much of the behavior of Object.create, Object.getPrototypeOf, Object.getOwnPropertyDescriptor, etc. work well currently, even if enumerable/writable/configurable cannot be set correctly. This would be lost.

And that's fine for it to be lost. Partial functionality doesn't belong in a shim (better suited for a utility function).

Object.freeze, Object.seal, and Object.preventExtensions have great applicability in secure JavaScript work like Mark Miller's. But for most of us, they're used to prevent mistakes during development or to send a message to the library consumer. Having them silently do nothing on legacy engines is not harmful since they are mainly development aids. Should we still cut them out?

Again devs will assume because the method exists that it works as expected.
Let devs decide how to handle situations where an ES5 method does not exist.

Even modern engines don't follow many aspects of the spec correctly, as evidenced by test-262 failures. Is it fair to demand es5-shim be better than they are? Is it helpful for es5-shim to be incompatible with them? Should es5-shim test for spec violations and try to replace the implementation where possible? Should es5-shim test for spec violations and remove the native implementation if it does not conform? I believe this has come up several times already with regard to date parsing, actually.

Yap engines have edge case problems with several ES5 methods, however that's not an excuse to implement partial or non-working shims.

As an example of the last, somewhat-recent versions of JavaScriptCore (possibly current ones too) give Object.create(null).hasOwnProperty("proto") === true. If, as this issue suggests, es5-shim's goal is to bring all engines in line with the ES5 spec or die trying, then it should remove Object.create from JavaScriptCore.

As I said earlier I am concerned with core functionality not edge cases. However I would not be opposed to nulling Object.defineProperty in browsers like IE8 and others that only work on DOM elements (host objects) or cannot achieve basic functionality. I am indifferent to ES5 methods that don't work well on DOM objects (Safari's Object.defineProperty for example) as host objects have always been a mixed bag (including the transition from ES3 to ES5 host object rules).

I would think that removing unsafe shims (and probably being more rigorous about determining which those are, perhaps by running them through the test-262 suite) is the job of a fork.

As it is es5-shim is less than ideal as production ready JS (unless the goal is to break dev expectations and/or code that relies on working shims). I would probably remove the .js and make this repo just a gist of JS snippets for devs to pluck from.

[DavidBruant]

I think that there are 2 types of population to distinguish:

1. Some people want "if(Object.create)" to be a sign of existence and full ES5.1 conformance (existence equivalence to full conformance). For these, if a shim differs from ES5.1, it should signal it by:
   1.1) at runtime throwing an error (maybe creating a NotSupportedError could make sense here) or;
   1.2) not support the features at all (absence of the function)
2. Other people want a library to integrate to their project without caring about limitations and just expect from the lib to do its best to conform but shut up when they diverge from the spec.
   2.1) Sometimes, people really care about full conformance when possible
   2.2) Sometimes, they care about the main use cases (for Array.prototype methods, some people don't care about skipping holes because they write and use dense arrays, so they want the performance instead of the checks required by the conformance)

If there is one thing I'm sure of it is that no library should do a mix two or three or four of the different tastes.

My understanding is that kriskowal/es5-shim/ chose path 2.1) (or 2.2?). No biggie, that's a choice.
But I understand and agree about the need for a library for 1.1 and 1.2.
This is probably one of these cases where a fork (or 2 actually in this case) may be needed, because despite apparences we're talking about different libraries.

[kriskowal]

Suffice it to say, there isn’t one right way to do this and I have no imagination that ES5-Shim is the one-true-library. I do not intend to try to make it the one-true-library. I do hope that this library will be a good parent, providing solid reference implementations of these methods that are good enough in some imperfect circumstances.

Different people need different things. This library is very clear about what it is and isn’t, with thanks to @allenwb in #4 for making sure that happened.

One of the gifts I want this library to continue giving is permission to use Object.freeze and Object.seal. Without stubs for these methods, code cannot be written that is agnostic to whether it is being used in a security sandbox. There are similar reasons for keeping other no-op and partial-op stubs. Perhaps we need branches for different needs.

I have definitely noticed the tension: some people like @jdalton, and most of this library’s active supporters, call for closing more and more edge cases. Others, like @kriszyp, call for relaxing a bit and breaking it up for better performance. It is impossible to satisfy both extremes with something as simple as this library.

There are variants of ES5-Shim that cater to fewer edge-cases, follow the specification less closely, and are smaller and faster. Some people need these.

Some people need bits and pieces of ES5-Shim. They are content to take them manually. We can make that easier by creating links to each method on Github from our README.md, or @jdalton, you might just go make that Gist; it’s not a big difference in approach. It would be better to automate this. Everyone needs that, but when it exists, practical limitations will prevent the majority of users from taking advantage of it.

A worthy project would be an entire system based on feature detection and systemic analysis that downloads and applies shims based on whether they’re both not provided by the environment and also needed by the code that is being loaded. @kriszyp has called for such a system. I’ve heard Peter Higgens, Alex Russel, Mike Samuel all call for some piece of this puzzle.

Another approach would be to use automatic dead-code-elimination. Google Closure Compiler can do this, but it has to operate on an entire working set. @cramforce has recently done some work there to make CommonJS and RequireJS compatible with dead-code-elimination with Closure Compiler. ES5-Shim would probably lose a lot of weight in many situations, especially if the code eliminator were advised of what methods were and were not provided by a particular client, perhaps based on feature-detection fingerprints.

However, the most cost-effective solution is to chill and wait for browsers to catch up.

@jdalton is right that we need more, but most of these things we need don’t need to be provided directly by ES5-Shim. I am happy to direct users away from ES5-Shim if it is not what they need.

[Raynos]

@jdalton

Again devs will expect because the method exists that it works as expected.

Incompetent developers who do need read the documentation or understand that the project uses the ES5-shim. We shouldn't protect against such incompetence. "Stupid people might misuse the tool" is not a good reason.

As it is es5-shim is useless as production ready JS

Not true at all. I use the es5-shim in production every day. I know the limited subset of ES5 that can be shimmed and I only code in that subset. I know the edges at which the es5-shim breaks and I accept them.

ES5-shim is production ready, you just need to learn to use it.

I defiantly see your problems, but you need to remember some people are happy with what the ES5 shim provides currently.

[jdalton]

@Raynos

Incompetent developers who do need read the documentation or understand that the project uses the ES5-shim. We shouldn't protect against such incompetence. "Stupid people might misuse the tool" is not a good reason.

As I stated earlier lots of popular libs, not supid people, use these weak inferences.

@kriskowal

By implementing partial or no-op shims es5-shim makes it hard to work w/ 3rd-party JS as most will not consider the cluster of gotchas that es5-shim forces on the developer. Partial implementations belong in utility methods not exposed on Object.

Cross-browser use is also mangled as some functionality will work in a browser/version but throw errors in other browsers/versions. Would be nice to be able to detect non-working shims without throwing errors (maybe by not defining a partially working shim in the first place).

One of the gifts I want this library to continue giving is permission to use Object.freeze and Object.seal. Without stubs for these methods, code cannot be written that is agnostic to whether it is being used in a security sandbox. There are similar reasons for keeping other no-op and partial-op stubs. Perhaps we need branches for different needs.

So for this gift you burden devs w/ more complex feature checks to ensure that the method isn't simply faking a no-op, ugg.
Devs should be allowed to decide how they want to fork, globally exposed noop methods are just dev land mines.

Others, like @kriszyp, call for relaxing a bit and breaking it up for better performance. It is impossible to satisfy both extremes with something as simple as this library.

Relaxed conformance/partial implementation is fine if it's a utility method that isn't exposed on native prototypes or Object.

[millermedeiros]

false positives is a big problem, I agree that partially shimmed behavior is a bad thing and it should be removed to avoid headaches... errors should "explode in our face" and never be silent, easier to find where the problem is...

[robotlolita]

@jdalton

Sorry, but I can't see your point. You're saying that libraries and frameworks should account for the presence of shims, but they obviously shouldn't. Shim libraries are things end-users, the ones who decide which frameworks and libraries they need to work with, decide whether to use a shim or not, and all that based on the requirements for the libraries they have decided to use.

If a library decide to rely on a shim, then it's broken. That's all there's to it. Libraries that should support different platforms should make it explicit to the user -- by saying on the documentation the preconditions, what they expect from the environments, which environments they support, etc. they shouldn't, however shim things that are not related to the problem the library solves at all (which is what I believe jQuery, Prototype, etc does). That's what causes problems: libraries trying to solve more than they're meant to. That makes such libraries non-composable, and that's never a good thing.

Now, given es5-shim is a end-user library, it doesn't matter if it provides partially shimmed methods or not. The only thing that can happen with this decision is that all-too-greedy libraries may be incompatible with it. That's okay, es5-shim, as pointed before, doesn't solve all the problems. It solves only one, and it's pretty boldly stated in the README:

Provides compatibility shims so that legacy JavaScript engines behave as closely as possible to ES5.

This package requires quite a bit more attention and testing. It is not likely to behave as advertised in a large cross-section of browsers.
...
Many of these shims are intended only to allow code to be written to ES5 without causing run-time errors in older engines. In many cases, this means that these shims cause many ES5 methods to silently fail. Decide carefully whether this is what you want.

Now, as said before, removing the partially shimmed methods would be a change of philosophy, and thus the work for a fork. It would be nice having a collection of safe shims readily available for those who want it, but es5-shim definitely doesn't fit that bill :3

[kriskowal]

There is only one rational way to solve this.

• https://github.com/kriskowal/es5-shim Will adopt a new charter. I will accept @jdalton’s patches to remove (or rename/factor) dubious shims. These will be released in a highly advertised major version and folks will be encouraged to switch camps if they want to keep dubious shims.
• https://github.com/kriskowal/shimsham Is a fork of ES5-Shim that will continue to have dubious shims. Bound by looser laws, it may also relax on spec compliance, and may incorporate shims beyond the charter of ES5 shimming, like setImmediate, de-facto standards, and what not.

Bug fixes can be cross-merged.

Please form teams.

[jdalton]

@killdream

Sorry, but I can't see your point. You're saying that libraries and frameworks should account for the presence of shims, but they obviously shouldn't. Shim libraries are things end-users, the ones who decide which frameworks and libraries they need to work with, decide whether to use a shim or not, and all that based on the requirements for the libraries they have decided to use.

I think you misunderstood. I'm saying libs/frameworks do property inference for using native es5 methods and fork their code accordingly. Partially working or noop shims throws a huge wrench into that.

Now, given es5-shim is a end-user library, it doesn't matter if it provides partially shimmed methods or not. The only thing that can happen with this decision is that all-too-greedy libraries may be incompatible with it. That's okay, es5-shim, as pointed before, doesn't solve all the problems. It solves only one, and it's pretty boldly stated in the README:

If es5-shim is included before other libs/code it could cause problems because the libs will infer that a method exists and could attempt to use it in a way not supported by the partial shim.

Now, as said before, removing the partially shimmed methods would be a change of philosophy,

It should be changed ;D

[Raynos]

Do we need two projects for this?

Why can't we just place dubious shims behind a little gate where we have to invoke a method or set a global flag to have them injected into global objects?

I think splitting the project into two is going to cause unnecessary divergence.

[Aside: I also vote we make @jdalton idea the fork >_>]

[jdalton]

@kriskowal

If shimsham still extends native prototypes and places methods on Object then it's still a problem. Polluting the global with non-compliant-half-working shims is the very thing you should be trying to discourage.

[kriskowal]

I actively dislike it when technical issues are elevated to moral issues. I am not closing this issue, but I am disabling notifications. Attach a pull request.

[jdalton]

Closing as a pull request is here.

[medikoo]

The best solution will probably be to allow users to import only chosen shims not whole at once.
What's brought up here, might indeed be an issue in some environments.

[jdalton]

@medikoo yap, a pull request is in the works to do something like that too :D

[medikoo]

@jdalton on my side I keep each utitlty function as separate module (Node.js style), then for browser I pack needed stuff with this https://github.com/medikoo/modules-webmake