search

chaplinjs / chaplin

HTML5 application architecture using Backbone.js
http://chaplinjs.org
Other
2.85k stars 231 forks source link

Don't overwrite containerMethod #887

Closed Florian-R closed 8 years ago

Florian-R commented 8 years ago

When a view as an option who is named like a jQuery method (for exemple filter), containerMethod take this value and the original value for containerMethod is lost.

This branch of the code was introduced during #877, @shvaikalesh can you expand on it? I assume this is for :

Handled the case when View is constructed with { 'after' } as an options. Test for this was present but reported incorrectly.

but I've never seen this options in the docs, and test are still green without this branch of code.

Edit: Hadn't seen this failure locally, investigating.

Florian-R commented 8 years ago

So if I understand correctly:

This test currently fails. It replaces this one

This test check for an unspecified behaviour (passing after as an option instead of containerMethod).

Reverted the test to the previous version.

shvaikalesh commented 8 years ago

I have added this behaviour because of the test. I do not think anyone uses it, and would rather drop another jQuery check from Chaplin core. LGTM.

Florian-R commented 8 years ago

I do not think anyone uses it, and would rather drop another jQuery check from Chaplin core.

FWIW, I've tried in some branch (https://github.com/Florian-R/chaplin/commit/26d42db3dbbde2070a0aa686c9796ae6dc1c2189) to remove the branching in attach, but it demands to recreate so many methods from jQuery with potential differences than I doubt it worth doing it.

Problem is that the actual behaviour makes code designed to work with jQuery breaks without and vice-versa.

No idea on how to fix this.

shvaikalesh commented 8 years ago

We should not recreate this methods, they are part of DOM API LS. Users should use polyfills. However, this functionality is not documented and Chaplin is used primarily in legacy apps with no cutting edge standards in mind.

Otherwise, I would check for methods, but it would negatively affect performance (every key in options should be looked up in Element.prototype). And yes, there are minor differences, but if user installed the polyfill he should be aware.

@paulmillr, maybe we should support DOM4 stuff?

Florian-R commented 8 years ago

@paulmillr Friendly ping

paulmillr commented 8 years ago

I don't think the IE versions we support can use dom4

Florian-R commented 8 years ago

This branch don't use dom4 at all. It just revert an incorrect behaviour.

Florian-R commented 8 years ago

Thanks!

Florian-R commented 8 years ago

@paulmillr If you have some time, could you publish a new release? Thanks!

bharatpatil commented 8 years ago

👍

paulmillr commented 8 years ago

done