Plugins' namespaces' usage - Githubissues

IITC-CE / ingress-intel-total-conversion

intel.ingress.com total conversion user script with some new features. Should allow easier extension of the intel map.

https://iitc.app

ISC License

284 stars 110 forks source link

Plugins' namespaces' usage #46

Open johnd0e opened 5 years ago

johnd0e commented 5 years ago

I have several conceptual questions regarding functions naming etc. So the 1st:

In all official plugins I see the same pattern of long-and-ugly namespace references, like window.plugin.showLinkedPortal.previewOptions (it's also inconsistent as somewhere there are references w/o window. prefix).

Shouldn't we fix it, like here: https://github.com/IITC-CE/ingress-intel-total-conversion/commit/c1aec193c74800c0258c29d39cb09fa6422966fb?

johnd0e commented 5 years ago

Is there a reason to have namespace as function-object? Why do not write just
```
var self = {}; // instead of function() {}
```
?

johnd0e commented 5 years ago

Why we ever need those 'namespaces'?

I can imagine a reason: for monkey-patching plugin's internal functions, in order to customize or extend them. Don't I miss other possible reasons?

If I don't, that should we expose in namespace every internal function? Wouldn't be better something like this: https://github.com/IITC-CE/ingress-intel-total-conversion/pull/40/commits/4e9ca02cfa4565496f10950030ecaab0f428dad0?

johnd0e commented 5 years ago

As far as I know (and it's not much as js is new to me), functions' declarations are often preferable then anonymous functions, e.g. because in debugger stack view we can easy distinguish them by names.

So wouldn't be better do like this: https://github.com/IITC-CE/ingress-intel-total-conversion/pull/40/commits/9ab75a8548b6a02ae94abc11ad74a29fbdb34e14?

modos189 commented 5 years ago

I've seen your similar question in the parent repository, but I don't have an answer as to why it's done this way. If the work does not break in browsers and does not violate backward compatibility with third-party scripts, I think you can safely change

johnd0e commented 5 years ago

Well, it definitely does not break anything in my experiments, but my testing is rather limited..

I think we should not make such changes globally yet (at least until #2 is completed). I propose to consider this issue as step to general future code clean up.

Meanwhile, if there are no objections, I could apply this locally, to that parts of code that I am rewriting now anyway.

modos189 commented 5 years ago

Agreed. I will carefully test and if something breaks, I will inform

johnd0e commented 5 years ago

I found possible explanations. of that long function references.

On exception there is a message in console, with these over-referenced functions' 'longnames', specifying place of error (so no need to open debugger).
(still no clue)
In order to see all functions 'longnames' in the console we need to put every of them (!) into that namespace.
Declarated functions always shown with shortnames in console.

I have understood the reasons, but still not accepted them, as it is really ugly. Errors occurs not so often, and I better will use debugger to see the their source.

What do you think?

johnd0e commented 5 years ago

In my opinion, for plugins we should avoid such namespace usage.

some complex plugin still may need namespace in order to be extendable by 3rd-party plugins (via monkey-patching).
simple plugins do not need namespace at all, see here: https://github.com/IITC-CE/ingress-intel-total-conversion/pull/40/commits/7639fbc970b826353363ef6f13e25d2e9fa35a5c (imho sources got a lot cleaner)

MysticJay commented 3 years ago

As a PoC #445 implements all functions as local functions and explicitly exposes some of them.