search

intercellular / cell

A self-driving web app framework
https://www.celljs.org
MIT License
1.5k stars 94 forks source link

moved NodeList.prototype.forEach out of God #26

Closed fprijate closed 7 years ago

fprijate commented 7 years ago

forEach polyfill doesn't belong to God.

gliechtenstein commented 7 years ago

Hi, thanks for the PR!

Just to explain why I put it inside God, my intention was that God is the initializer. That's why it takes care of the overrides such as forEach and the next line where it overrides DocumentFragment and Element.

Could you share your point on why it doesn't belong inside God? Would love to discuss. Thank you!

fprijate commented 7 years ago

Hi, thanks for your answer.

The only polyfil in God, I see is forEach polyfill. IMHO polyfill just implements some method in browser which isn't implemented yet natively (or has some bug in implementation), and has nothing in common with application logic. Usually we use some polyfill library or transpiler to implement it.. I think that polyfils should be outside application logic.

The other two (DocumentFragment.prototype.$build and Element.prototype.$build), in my opinion arent polyfills. They are introducing new $build method which is deeply connected with Gods role of creator.

fprijate commented 7 years ago

Hi

 if (window.NodeList && !NodeList.prototype.forEach) { // NodeList.forEach override polyfill

"Override" is wrong in comment. It doesn't overrides anything , but introduces missing method in browsers not implementing it (Edge). In the case, of changing functionality in all browsers (so overriding it,which is bad) , it would belong to God.

gliechtenstein commented 7 years ago

Thanks for the explanation. I also appreciate your explanation over at https://github.com/intercellular/cell/issues/29

You've convinced me on both points, and I think we can change this. But one thing though.

While I agree with taking the polyfill out of God, I don't think we should put it at the top, but right after God. https://github.com/intercellular/cell/blob/develop/cell.js#L374

I think it makes more sense there based on your explanation. What do you think?

Also I think we can also incorporate your suggestion about using Array.prototype.forEach instead because it's shorter. If something ends up breaking because of this (although not likely) I guess we can fix it then.

If you're fine with all this, could you update the code to reflect this and recommit? I'll merge it in.

Otherwise, please let me know. Thank you!

devsnek commented 7 years ago

with pr #112 it would be best to leave this in god.create to correctly scope things when applying the polyfill

gliechtenstein commented 7 years ago

Yup, with https://github.com/intercellular/cell/pull/112 now it makes more sense to keep it in context so let's keep it here for now.

Even though eventually not merged, I appreciate the PR! I think it's really important to take these structural things seriously especially for Cell due to its minimal nature, and i'm glad that i spent some time thinking about this through this PR 👍