iitc-project / ingress-intel-total-conversion

ingress.com/intel total conversion user script with some new features. Should allow easier extension of the intel map.
http://iitc.jonatkins.com/
ISC License
991 stars 552 forks source link

Questions about plugin structure #1298

Open johnd0e opened 5 years ago

johnd0e commented 5 years ago

https://github.com/iitc-project/ingress-intel-total-conversion/blob/f90452765baf1baffd918117598ef9becd41db87/plugins/default-intel-detail.user.js#L26-L37

  1. There is anonymous function in assignment: window.plugin.defaultIntelDetail.setup = function(). But would it be better to use functions names in order to see them in debugger?

  2. Why do we ever need own namespace for plugin? I can understand if there is demand for cross-plugin data access, but here is definitely not that case.

  3. Why do we ever need to store private functions in namespace? Why do not write just like that:

    
    function setup() {
    //
    
    window.CONFIG_ZOOM_DEFAULT_DETAIL_LEVEL=true;

};

Eccenux commented 5 years ago

If you look at the code of any plugin you will notice that each plugin is in a function. Code of the plugins is not executed at once. It will wait for IITC to load.

There is also some integration with GM_info which gives you script information.

Namespaces are important as there are many plugins out there. But you don't have to create your namespace if the plugin is to be fully private (no public methods nor properties).

johnd0e commented 5 years ago

@Eccenux, more discussion here: https://github.com/IITC-CE/ingress-intel-total-conversion/issues/46

you will notice that each plugin is in a function

Yes, and I try to understand the reason: why it's function?

// It would be enough to define it as plain object literal:
 window.plugin.defaultIntelDetail = {};

And even more, it would be cleaner if defined like this:

var defaultIntelDetail = {};

// we can omit `window` part of `window.plugin.defaultIntelDetail`
plugin.defaultIntelDetail = defaultIntelDetail;

// this is shorter that `window.plugin.defaultIntelDetail.someAction`
defaultIntelDetail.someAction  = function() {
   // ...
};

There is also some integration with GM_info which gives you script information.

Could you explain further? I saw that part of iitc code, but still unable to understand.

Eccenux commented 5 years ago

you will notice that each plugin is in a function Yes, and I try to understand the reason: why it's function?

For both isolation and delayed execution (interpretation) of the code. Note that just below user script template you have GM_info available, and inside the wrapper the context changes. The code is run in context of the site not in a Tampermonkey context.

Did it really had to be done like that?... I'm not sure. TM does provide some isolation, but then you might need to use unsafeWindow and stuff like that...

johnd0e commented 5 years ago

Actually I asked another question:

 // use own namespace for plugin 
 window.plugin.defaultIntelDetail = function() {};

Why is it a function? Why not plain object?

 // use own namespace for plugin 
 window.plugin.defaultIntelDetail = {};

For both isolation and delayed execution (interpretation) of the code.

You are talking about wrapper purpose. I completely agree about 'delayed' execution, as setup function should not be run until iitc framework is ready.

The code is run in context of the site not in a Tampermonkey context.

Here I do not agree, and for Tampermonkey we can completely omit the part after wrapper ('inject code into site context'). You can try yourself: https://github.com/IITC-CE/ingress-intel-total-conversion/pull/51. But we still need that for Greasemonkey.

Eccenux commented 5 years ago

Why is it a function? Why not plain object?

Not sure what you are asking here... It's just a convention I guess. Technically you don't need that at all. You can just write a class and export objects you need or export nothing... But when you don't export anything then other plugins might not be able to detect your plugin and plugins will not be able to interact with each other (like e.g. sync and uniques and bookmarks). Example of exporting a function when you need to: https://github.com/Eccenux/iitc-plugin-data-merger/blob/v0.3.0/data-merger.user.js#L170

But we still need that for Greasemonkey.

Greasemonkey is dead (died with introduction of FF Quantum). It's not supported in new browsers AFAIK.

The wrapper is actually needed for backward compatibility. You will probably break other plugins if you start removing stuff like that.

Also note that your assumptions in https://github.com/IITC-CE/ingress-intel-total-conversion/pull/51 are incorrect. Plugins don't polute global namespace unless you make them. This is not how it works. Try to run wrapper in console. It won't work, definitely not with Tampermonkey, but I'm quite sure the same was true for Greasemonkey.

Sorry, but are you sure you have enough experience with JavaScript and user scripts to mess with how ITTC works? This is a delicate matter. I alone have more then 10 custom plugins and I know there are more out there.

johnd0e commented 5 years ago

It's just a convention I guess.

Well, I am trying to find a point why it is like that.

But when you don't export anything then other plugins might not be able to detect your plugin and plugins will not be able to interact with each other.

Look again at defaultIntelDetail namespace, there are two cases, which are equivalent in usage. Both of them allow to detect/interact. So I suppose that we do not need 'functional' namespace, as plain object is enough, right?

Greasemonkey is dead

Shouldn't we still support legacy Firefox users?

You will probably break other plugins if you start removing stuff like that.

I am not going to remove anything until I'm make sure that it's safe. That's why we discussing it. As for wrapper: nothing breaks in my tests. Point me out if I've missed anything.

Plugins don't polute global namespace unless you make them.

Open console and see wrapper, script, info. Are they part of API? No, they are pollution, side-effect of wrapper. (Not a big deal though, as there are only 3 of such objects)

Sorry, but are you sure you have enough experience with JavaScript and user scripts to mess with how ITTC works?

I'll do my best, thank you. And you could participate if you are interested. I use some of your plugins and appreciate your experience with iitc.

Eccenux commented 5 years ago

Sorry for my attitude. Maybe I'm just getting nervous when I see someone is about to break backward compatibly.

It's just a convention I guess. Well, I am trying to find a point why it is like that.

Don't know what was the original intention, but I'm quite sure it is not used (I have uniques coloring plugin that don't define it). I'm also quite sure you will never know the original intention... because it's not documented ;-).

If you ask about technical difference. Then when you define a function you are defining both an object and a class. Yes, the same class you can actually define with class keyword now.

Open console and see wrapper, script, info. Are they part of API? No, they are pollution, side-effect of wrapper.

Which console do you refer too? I cannot reproduce this. console_ff_chrome

To my knowledge user scripts were always sandboxed. Maybe you misinterpreted something?

Note that the wrapper actually both breaks a sandbox created by TM and creates it's own sandbox. The code around the wrapper is actually quite interesting when you analyse it fully. Though I would agree it looks messy and could definitely be documented better.

johnd0e commented 5 years ago

Which console do you refer too? I cannot reproduce this.

You can see it in mobile console. And it seems mobile-specific issue, thank you for pointing this out.

The code around the wrapper is actually quite interesting

Sure it is, though now in Tampermonkey we could keep things simpler. But I care about compatibility, and not going to breaks anything without strong reason.