search

microsoft / ace

Build Cordova apps with true native UI
http://microsoft.github.io/ace
Other
850 stars 157 forks source link

JS prototype extension issues #66

Open davidgovea opened 7 years ago

davidgovea commented 7 years ago

Hello!

First off - let me thank everyone involved in this project. It's amazing! The universal native bridge portion alone is a golden egg.

I'm building cordova apps with Ember, and was seeing some strange breakages that are being caused by the javascript prototype extensions that ace uses colliding with some behavior of Ember. I was able to temporarily work around them for now by simply dropping Extensions.js from the cordova_plugins.js manifest (not using any features that depend on the extensions)

screen shot 2016-08-15 at 3 33 25 pm

Now, I realize that this problem is not a fault of ace by itself - Ember's prototype extensions are ..extensive.. (wayy more evil there than in Extensions.js), and we're on the path towards removing them from our projects entirely (Ember provides alternate ways to use the utility functions).

After browsing the code, it doesn't seem like these extensions are used in many places, and in some cases might be there to simply provide a more "java-like" syntax (though I could be mistaken).

tldr; I wanted to see how the maintainers of this project would feel about moving away from JS prototype extensions. I'd be happy to contribute a branch for review, if "no extensions" sounds like a desirable goal for maximal compatibility with various projects.

In the meantime, I'll be profiling the actual conflicts more in-depth (sorry for the lack of details!), and perhaps starting to tinker with the no-extensions refactors, but I don't want to be maintaining a personal fork if you all disagree (would probably approach it from the ember side in that case, since a supported path already exists).

Thanks for reading, and thanks again for this project!

ihadeed commented 7 years ago

I'm not a big fan of prototype extensions either.

+1

davidgovea commented 7 years ago

So, I went ahead and made a branch to test out the refactors: https://github.com/runtrizapps/ace/tree/prototype-ext-refactor

I was able to solve my issue (it was a conflict with Array.prototype.removeAt).

I refactored "in place" by replacing calls like data.removeAt(1) with ace.Extensions.removeAt.call(data, 1)

and then did some further refactors to removeAt(data, 1) by storing a local reference to ace.Extensions.removeAt.call

I still need to verify my changes further (and run tests if available?) - I should have some time later this week to do a proper PR and writeup.

EDIT - my branch has some serious issues. Was trying to get fancy with .call() and it just doesn't work. Will be reverting my last commit before continuing. No need to review the branch at this time, but I'd still be interested in any "no extensions" commentary from project maintainers. Thanks!

adnathan commented 7 years ago

I think it's reasonable to move away from any extensions that are problematic. So feel free to submit a pull request. Thanks!