stampit-org / stampit

OOP is better with stamps: Composable object factories.
https://stampit.js.org
MIT License
3.02k stars 102 forks source link

Use a ponyfill instead of using Object.assign directly, to support IE11 #320

Closed Mustack closed 7 years ago

Mustack commented 7 years ago

Fixes https://github.com/stampit-org/stampit/issues/319

danielkcz commented 7 years ago

Thanks for the effort, but we have decided a long time ago that polyfill for this won't be bundled with stampit, especially since only the IE11 is affected by it. In the install section there is an explanation how to handle this.

Furthermore, stampit itself is slowly getting deprecated and replaced by modularized approach residing in repo https://github.com/stampit-org/stamp/tree/master/packages. Not that it would solve your problem in any way, but there is not much of the point to send PR to this one.

jyboudreau commented 7 years ago

Hi @FredyC,

I agree that this can be fixed via a polyfill and this is likely a valid use case for application developers. However, using a polyfill is not a valid use case for library developers such as yourself and others.

As such any library developer wanting to use stampit or @stamp packages have to either pollute globals or pass along this problem up the dependency chain.

When such a simple solution exists (~20 lines of code) why restrict your audience to people who:

danielkcz commented 7 years ago

I think that even on library level it makes sense to include this ponyfill that simply checks if Object.assign is available and if not, it would include it. I wouldn't consider that polluting globals as you are only adding functionality by spec, nothing that shouldn't be there. Or do it configurable and allow a user of your library decide if they want to use it or they have own solution in place.

We wanted stampit as small in size as possible, so even including 20 lines just for a single use case feels like unnecessary evil.

jyboudreau commented 7 years ago

I also think these are valuable goals. They are valuable goals for my own libraries as well, but now if I want to use \@stamp\stampit I have to pass along that problem.

Or do it configurable and allow a user of your library decide if they want to use it or they have own solution in place.

Why not provide this configurability in \@stamp\stampit then? I.e. Allow users to provide their own assign function.

danielkcz commented 7 years ago

@jyboudreau Such configurability is not that easy on our level as we would need to create separate bundles that would include it. Sorry, but IE11 is not worth the effort :) If you really want it that way, you can try to create PR, but I doubt it would be useful.

Comparing goals of your library and stampit doesn't seem right. On one hand, you would want your library to be small (if I understood correctly?), but at the expense of increasing the size of its dependencies? Including the fact that size would be bigger for everyone even people who don't need it. I fail to find the logic in it :)

jyboudreau commented 7 years ago

@FredyC Good points. We'll look at our options and might provide a PR for configuration.

Right now since we're bundling with WebPack and Babel I believe we are able to transform the global assign polyfill into a non-global one. Thanks for your help.

koresar commented 7 years ago

Without an internal Object.assign reimplementation: Estimating dist/stampit.umd.min.js: 5.16 KB, GZIP : 1.8 KB

With it: Estimating dist/stampit.umd.min.js: 5.26 KB, GZIP : 1.84 KB

The implementation look like this:

export const assign = Object.assign || function assign(to) {
  for (let s = 1; s < arguments.length; s += 1) {
    const from = arguments[s];

    for (const key in Object.keys(from)) {
      to[key] = from[key];
    }
  }

  return to;
};

I know it's unsafe, but works for the internal object merging.

I would rather release stampit v3 with this thing, but we should clearly state that "IE11 unit tests where never run, we do not have an ability to run stampit tests in IE11, use on YOUR OWN RISK...".

(Also cross posted to #319.)

koresar commented 7 years ago

Something only I know (sorry @FredyC I should have told you, my bad) is that the compose() standard does not require a super robust Object.assign implementation. So, the little internal "polyfil" from above works just fine under any corner case conditions.

This PR is fine but introduces a dependency which, as @FredyC pointed, is something we strongly advocate against.

So, I put up the new PR #321 which adds the internal "polyfil" from above.

UPD: @Mustack @jyboudreau You are not the first but third or fourth people who came across the polyfiling issue. So, I believe we should listen to the community and have the assign function built-in.

ericelliott commented 7 years ago

I disagree with the conclusion of this thread, and the logic behind it.

  1. Every app should be responsible for supplying their own polyfills for functionality because this issue exists in every library. If every library tried to solve it for every app, it would significantly bloat every download bundle with redundant code. It isn't a big difference in this one library, but multiply that logic by thousands of libraries, and it's like one more bad person throwing old plastic containers on the highway -- eventually, the entire landscape is cluttered with superfluous code. Stampit should be a good citizen and refuse to add to the problem.
  2. You don't have to "pass the problem" along to all your users if you are writing a library. If you like, you can decide to be a bad citizen and do the polyfill in your library -- but I don't recommend that. Instead, why not list known polyfill dependencies? That's a PR worth considering.
  3. Polyfills can be completely automated in an app with a single line to a polyfill service, with CDN capabilities and automatic browser caching baked in. Taking advantage of browser caching and CDN for polyfills is impossible if every library includes their own polyfill. Just add a couple lines to your docs, add a troubleshooting section documenting what to do if you get Object.assign is not a function errors, and call it a day.
koresar commented 7 years ago

Sorry @ericelliott but I have to disagree too few points you made.

1)

because this issue exists in every library

This is untrue. Huge exaggeration. Not in every, but in some.

2)

significantly bloat every download bundle

In our case, as you have read above, it's 40-100 bytes. I find these bytes quite far from significantly. Thus, again, your statement is untrue for stampit.

3)

Polyfills can be completely automated

In my experience in 90% cases this is simply impossible due to project specifics. In fact, you are suggesting their widget to depend on a third party service. That additional request will slow down the app because JS won't start executing until things are polyfiled.

4) The core misconception is the word "polyfil" here. Strictly speaking stampit does not need polyfiling. It needs a very simple object merging function, not the fully fledged Object.assign.


The most important here, @ericelliott , is that you are currently pushing toward stampit being a bad citizen. Why I think so?

Because IIRC there were at least 4 different people asking for that feature. This is a very high demand as for stampit-org. Trust me. I follow all of them.

Take into account that IE11 End Of Support is in 2025!

We must go towards the community, not against it.

ericelliott commented 7 years ago

Fine. It's small. Do it.

FYI, mainstream support for IE11 ends on October 13, 2020. After that it goes into "extended support" and will only receive patches for critical vulnerabilities.

ericelliott commented 7 years ago

Fun bit of trivia: An earlier version of Stampit included a ponyfill for Object.assign() and we removed it because the community requested its removal.

You just can't win this game. ;)

danielkcz commented 7 years ago

@koresar You have some good points too, but I am not convinced that we should include something that has native support in many other environments. Doing this means that you will never be able to use any optimizations coming from native implementations.

Why it's a problem to do what I outlined above? Just have a separate bundle that would include dependencies and be exposed as stampit/compat. People can easily alias that in Webpack in case of need and it won't influence others.

koresar commented 7 years ago

I love your idea Daniel. I welcome you to implement that. It would be awesome! We could ditch my pr #321

On 29 Aug. 2017 18:53, "Daniel K." notifications@github.com wrote:

@koresar https://github.com/koresar You have some good points too, but I am not convinced that we should include something that has native support in many other environments. Doing this means that you will never be able to use any optimizations coming from native implementations.

Why it's a problem to do what I outlined above? Just have a separate bundle that would include dependencies and be exposed as stampit/compat. People can easily alias that in Webpack in case of need and it won't influence others.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stampit-org/stampit/pull/320#issuecomment-325599399, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjCLw_52e22sGZ8_wdP1N5crt0A_-Omks5sc9GMgaJpZM4PEsgB .

Mustack commented 7 years ago

Thanks to all for your consideration on this issue. I can tell you that after further troubleshooting, I found another dependency of my lib used native promises. This meant I needed a more scalable solution than opening PRs in each project. I ended up using babel transform runtime to solve this. I've also been able to remove the ponyfills that I was using in my own code.

If Stampit were the only dependency, using a "compat" version of stampit would probably be the most simple solution and I think I would have used that. I'm just sharing my experience for your consideration.

Thank you for the work you guys are doing on this project. It helps a lot.

danielkcz commented 7 years ago

@koresar I don't want to make any promises right now, I have enough stuff on my plate right now :)

@Mustack transform runtime can bloat the code quite a lot if not treated carefully. If you are using it for a lib transpilation a lot of code can be duplicated across various dependencies. It's probably not a solution I would go for.

ericelliott commented 7 years ago

The best solution I know for this problem is a polyfill CDN service. Advantages include:

No other solution can check all these boxes because other solutions require you to bundle the polyfills with your application code. Just to reiterate: That's a bad idea, and using polyfills/ponyfills in library code is littering.

danielkcz commented 7 years ago

@ericelliott I agree with you ... when you are developing an app. If you are lib author, then you can either include ponyfill by yourself or document that people need to use mentioned service. It's not exactly win-win scenario.

ericelliott commented 7 years ago

@FredyC That's not how libraries work. Libraries are multipliers. They have a multiplying impact on everything they touch. If you add a line of code, that's a line of code for EVERY APP using your library. You also have to consider that there are other library authors facing the same choices.

If you are a library author, it's your responsibility to consider how your decisions scale if all the other library authors are faced with the same choices (because they often are). So let's take a closer look at how this scales in practice.

Imagine the case where every library using ES5+ features (read: most modern JS libraries) decides to use polyfills or ponyfills rather than assume that the feature exists in the app. I'm looking at a small app right now (started a few months ago). There are 695 dependencies in node_modules. Imagine for a moment that 25% decide to implement 20 lines of ponyfill. Now there are 3,500 lines of code that don't need to be there -- in a tiny app where the total LOC for the app source code itself is barely more than 10k lines. We've added 30% to our bundle size because some library authors thought, "this is only 20 lines, I really should do this here".

(695 * .25) * 20 = 3475 WASTED LOC

Now multiply that impact by the 10,000 apps using your code: You've unleashed 200,000 lines of wasted code on the world.

Now imagine you're a large enterprise. You're building a flagship product. Your app has over 1 million lines of code. You have 15k dependencies. Your biggest problems are bundle processing and load times. You've worked very hard to carefully split your entire app into 200k bundles for fast loading and chunking. You decide to dig and discover that 25% of your library module authors thought, "it's only 20 lines, let's just include this harmless ponyfill":

(15000 * .25) * 20 = 75,000 WASTED LOC

Your litter is just one more plastic jug on the side of the highway, but if everybody thinks that way, pretty soon there will be garbage draping the sides of the highway as far as the eye can see.

This is what I mean when I talk about being a responsible citizen as a library author.

Imagine that for every star, there are 100 apps using your code. Imagine that our ponyfill is only 10 lines. Stampit has over 2,500 stars:

(2552 * 100) * 10 = 2,552,000 WASTED LOC

Starting to get the big picture?

This impact is the reason that library CDNs exist. It's the reason why polyfill CDNs exist.

If you want to just create a library that works, sure, polyfill the thing and forget about it.

If you want to be viewed as a good citizen by enterprise scale companies that really do have to worry about the impact of scale, and if you want to do your part to reduce wasted bandwidth and improve connection speeds for all the users of all the apps using your libraries, these are the facts you need to consider.

sethlivingston commented 7 years ago

Sounds like your next article, Eric. Good stuff.

koresar commented 7 years ago

Conclusion from Eric's comment: JavaScript needs a much better standard library.

I'm going to merge the PR #321 and publish v3.1.4 if there are no hard objectives in 24 hours.

thanks everyone for a very constructive discussion.

PS: yeah, @ericelliott , your comment sounds like great blog post!

jyboudreau commented 7 years ago

Great write up @ericelliott. I agree with others about this making a good blog post. I always like your articles.

With that said, I'm still only partially convinced.

In my case i'm developing a WebRTC library. WebRTC is still very much a moving thing. I can't exactly develop this library to the standard and then tell my users to fill in the gaps, I would have no users. A big part of any WebRTC library's responsibility at this point is to remove the difficulty of filling the gap from the user.

Since we take so much care to fill these WebRTC gaps for each browser it's then easy to extend that mentality to smaller things like Object.assign that have known and easy polyfills.

In other words, if we're going to advertise to our users that our library supports Chrome, Firefox, IE11 and Safari as a selling point then we'd better make sure it does work in all those browsers. It's already difficult enough to use WebRTC across different browsers without us making it slightly more difficult for our users.

Some parallels could be drawn to other libraries. Like JQuery, which I don't think would have been very popular if it just implemented everything to the standard at the time.

In the end I don't think being a good web citizen is as black and white as you paint it. There will always be some compromise between idealism and pragmatism.

ericelliott commented 7 years ago

@jyboudreau You make a great counterpoint to my argument, but WebRTC is a special case because there are few implementations, and the specs are in flux.

Something as well established as Object.assign() is a completely different matter -- the specs are not in flux, and there's a standards-compliant implementation in every modern browser.

If you want to polyfill stuff in your library that's already filling in a lot of browser gaps, that's fine, but if every library using Object.assign decided to polyfill it, that would add significant wasted space and bandwidth to virtually every JS app everywhere.

In other words, it's important to look at your particular use-case, and we're not letting users do that for themselves if we include a ponyfill for everybody by default.

koresar commented 7 years ago

Stampit v3.2.0 released. https://github.com/stampit-org/stampit/releases/tag/v3.2.0

TL;DR: We support all ES5-compatible environments now: IE11, node.js 0.*, IoT devices like puck.js, etc.

koresar commented 7 years ago

Last thought about the 100 bytes. :)

How much bytes (memory and CPU resources) is a HTTP request usually?

If 10,000 apps is using the library:

Scientific?

¯\_(ツ)_/¯