Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Questions about plugin structure #1298

Open
johnd0e opened this issue Dec 16, 2018 · 8 comments
Open

Questions about plugin structure #1298

johnd0e opened this issue Dec 16, 2018 · 8 comments

Comments

@johnd0e
Copy link

johnd0e commented Dec 16, 2018

// use own namespace for plugin
window.plugin.defaultIntelDetail = function() {};
window.plugin.defaultIntelDetail.setup = function() {
// NOTE: the logic required is closely tied to the IITC+stock map detail level code - so the logic is moved there now
// and just enabled by this flag
window.CONFIG_ZOOM_DEFAULT_DETAIL_LEVEL=true;
};
var setup = window.plugin.defaultIntelDetail.setup;

  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
Copy link

Eccenux commented Feb 15, 2019

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
Copy link
Author

johnd0e commented Feb 15, 2019

@Eccenux, more discussion here: IITC-CE/ingress-intel-total-conversion#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
Copy link

Eccenux commented Feb 15, 2019

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
Copy link
Author

johnd0e commented Feb 16, 2019

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: IITC-CE/ingress-intel-total-conversion#51.
But we still need that for Greasemonkey.

@Eccenux
Copy link

Eccenux commented Feb 16, 2019

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 IITC-CE/ingress-intel-total-conversion#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
Copy link
Author

johnd0e commented Feb 16, 2019

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
Copy link

Eccenux commented Feb 16, 2019

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
Copy link
Author

johnd0e commented Feb 17, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants