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

revise wrapper #51

Closed
wants to merge 2 commits into from
Closed

revise wrapper #51

wants to merge 2 commits into from

Conversation

johnd0e
Copy link
Contributor

@johnd0e johnd0e commented Jan 13, 2019

!Proof-of-concept branch
(to be merged after we'll make sure that there be no undesirable side-effects)

  • Do not try to 'inject script in site context' anymore,
    as it has no observable effect (if I haven't missed something).
    Thus wrapper is simpler now.
  • It's now real IIFE, and does not pollute global namespace with
    wrapper, script, info and so on.
  • No need to cut that 'inject' part of wrapper for mobile app.

@johnd0e johnd0e force-pushed the simplify-wrapper branch 2 times, most recently from 485fbde to f34a8f7 Compare January 13, 2019 11:03
- Do not try to 'inject script in site context' anymore,
  as it has no observable effect (if I haven't missed something).
  Thus wrapper is simpler now.
- It's now real IIFE, and does not pollute global namespace with
  `wrapper`, `script`, `info` and so on.
- No need to cut that 'inject' part of wrapper for mobile app.
@johnd0e
Copy link
Contributor Author

johnd0e commented Jan 13, 2019

I suppose the wrapper could be even more simplified.
Need to investigate how that info/plugin_info is used in other parts of code.

@AlfonsoML
Copy link

In my plugin for Pokemon Go, I avoided the usage of the wrapper and everything looked to work fine (and it's easier to work and debug because the code is run directly instead of reinjected as a script), and the only problem has been a recent report about not working with Greasemonkey that seemed to require this workaround due to the "unsafeWindow"
https://gitlab.com/AlfonsoML/pogo-s2/commit/137a0aff6e176fbefaf46f5799f687da5f307d62

@johnd0e
Copy link
Contributor Author

johnd0e commented Feb 2, 2019

I avoided the usage of the wrapper and everything looked to work fine

It's a matter of coincidence, as wrapper is really needed in order to:

  • run script when environment is ready (in your case Leaflet must be loaded before)
  • report metainfo to main script (for About window)

Try to reorder scripts loading order in Tampermonkey and see what may happen.

@AlfonsoML
Copy link

No, it doesn't matter the plugin order as the initialization is run in the "setup" function and it detects if iitc is ready or it should add itself to the list of pending plugins: https://gitlab.com/AlfonsoML/pogo-s2/blob/master/s2check.user.js#L3013
The wrapper is just old code for Greasemonkey and in order to run in the "main" context, but with Tampermonkey it isn't required (I think that it might be different if the plugin request something like GM_Grant priviledges, but not in my case and in this way I can debug it directly in the sources pane).

@johnd0e
Copy link
Contributor Author

johnd0e commented Feb 2, 2019

Ah, now I see what you mean.

as the initialization is run in the "setup" function and it detects if iitc is ready or it should add itself to the list of pending plugins

This part is exactly what I call 'wrapper', and it is followed by one more part, which is intended to inject code into site context.

The wrapper is just old code for Greasemonkey

Thank you for sharing this info.
May be we'll implement fix similar to yours.
Or just make separate option for build.py to be able to insert different wrappers.

I need to try Firefox.

@AlfonsoML
Copy link

Ok, I just called the 'wrapper' to the function that it's called 'wrapper' by default in all the IITC plugins 😀and that it's then use to inject the code again in the site context. I'm not talking at all about the "setup"

According to my tests, whenever people use Tampermonkey (with Chrome, Firefox or even Edge), then you don't need the hoop to reinject the code into the site context, but it turns out that it's required for Greasemonkey (which used to be the reference long ago, so that can explain why the code is structured the way that it is).

I would not create separate options to insert different wrappers, otherwise you end up with "install this for tampermonkey and this for greasemonkey" and the only difference are a few lines that can take care of this difference automatically

@johnd0e johnd0e added the WIP Work in progress || Proof of the concept label Feb 14, 2019
@johnd0e johnd0e mentioned this pull request Feb 27, 2019
@modos189
Copy link
Contributor

Perhaps, better I'll comment on here, not in #124

Page context and context extension (that loads UserScripts) different. This is a limitation of the browser sandbox.

To run the code in the context of the page, a wrapper is used.
Only Tampermonkey uses sophisticated hacks to make the default scripts run in the context of the page.
And I've noticed that some plugins also use this feature for their work.

Maybe it makes sense to abandon Greasemonkey and write simpler code without using a wrapper.
However, it should be noted that after version 2.9 Tampermonkey is not open source software. If Tampermonkey will implement a proprietary feature, missing in other managers scripts, should it apply, to depend on proprietary Tampermonkey?

@johnd0e
Copy link
Contributor Author

johnd0e commented Apr 15, 2019

And I've noticed that some plugins also use this feature for their work.

What exactly feature do you mean?

Maybe it makes sense to abandon Greasemonkey and write simpler code without using a wrapper.

  1. AFAIK we still need wrapper in order to delay scripts setup execution
  2. We are able to get rid only of a final part of wrapper: 'inject code into site context'
    But as I can see it may broke not only GM, but IITC-Button too.

May be it's possible to replace that 'injection' with some better technique, recommended for extensions.

to depend on proprietary Tampermonkey?

Well, there is also Violentmonkey, etc.

@johnd0e
Copy link
Contributor Author

johnd0e commented Apr 15, 2019

May be it's possible to replace that 'injection' with some better technique, recommended for extensions.

Actually I have no experience in extensions development, and do not know where exactly we really dependent on page context, and where it is just a matter of legacy code (you know, currently window object is greatly misused, #52).

So after we reorganize API (e.g. we could pass API object into setup function), may be there stays only small part of cases where we still would need context tricks, and we be able to manage it with some 'fair' methods.

@modos189
Copy link
Contributor

What exactly feature do you mean?

Use window object as if the script is already running in the context of the page.

For example, the ingressmosaik plugin in extension context get window.plugin.missions and window.dialog (which are available in tampermonkey but are actually in the context of page) and only if all is well then goes into the context of page.

it may broke not only GM, but IITC-Button too.

Yes, now IITC-Button works by Greasemonkey method

By the way, recently Firefox has a new API for UserScripts, but we need its support in Chrome

@johnd0e
Copy link
Contributor Author

johnd0e commented Apr 15, 2019

And I've noticed that some plugins also use this feature for their work.

No, that plugins not really use any feature, at least intentionally. I saw ingressmosaic plugin and can tell that it's author just 'reinvent' buggy substitution for iitcLoaded hook.
From the code I see that it can fail in Tampermonkey too in some conditions (just checked, really fails).

I don't feel that we should support plugins bugs. Better fix such plugins.
(Or, in case of unsupported ones, we could redistribute fixed versions ourselves)

By the way, recently Firefox has a new API for UserScripts

It does not seem to be something different from greasemonkeys's current, I mean there will not be lesser restrictions.

@modos189
Copy link
Contributor

modos189 commented Apr 15, 2019

Well. Then, if there will be hooks, we can continue to support GM

@johnd0e
Copy link
Contributor Author

johnd0e commented Apr 15, 2019

if there will be hooks

What hook do you mean?

@modos189
Copy link
Contributor

Ah, there's already has iitcLoaded. Okay

@johnd0e
Copy link
Contributor Author

johnd0e commented Apr 15, 2019

Yes, and I am even going to replace it with more reliable Promise (#76).

Edit: in most cases we do need iitcLoaded, as function setup called only after all the API got available.

@johnd0e
Copy link
Contributor Author

johnd0e commented Apr 16, 2019

Perhaps, better I'll comment on here, not in #124

Discussion about pure and fair GM support will be continued here: #124 (comment)

@johnd0e
Copy link
Contributor Author

johnd0e commented Apr 16, 2019

As for this PR:

  • We cannot use it in current form because of GM support
  • We can vary execution pattern like @AlfonsoML proposed revise wrapper #51 (comment)
    (I need to check if we really gain anything with that scheme)
  • If we want to change wrapper then we have to investigate more reliable ways of cutting/replacing it in IITCm.
    Perhaps better we leave old code for compatibility with old plugins.
    And it's possible that the best way will be to consider IITCm right in wrapper code (and do not try to cut it in Java code).

@johnd0e
Copy link
Contributor Author

johnd0e commented Apr 17, 2019

it may broke not only GM, but IITC-Button too.

I just realized that it's not true, as IITC-Button does not need wrapper at all.

Literally, it even can cut it out like IITCm does.
Related post: #124 (comment)

@johnd0e
Copy link
Contributor Author

johnd0e commented Oct 2, 2019

@AlfonsoML

I avoided the usage of the wrapper and everything looked to work fine (and it's easier to work and debug because the code is run directly instead of reinjected as a script)

Could you explain why it's easier to debug scripts without wrapper?
It seems that scripts are available is Sources pane in either case.

@johnd0e
Copy link
Contributor Author

johnd0e commented Oct 2, 2019

Oh, I've got it.
It's impossible to set breakpoints for 'reinjected' script.

@johnd0e johnd0e removed the WIP Work in progress || Proof of the concept label Oct 2, 2019
@johnd0e johnd0e closed this Oct 2, 2019
@johnd0e johnd0e deleted the simplify-wrapper branch October 2, 2019 14:36
@johnd0e
Copy link
Contributor Author

johnd0e commented Mar 7, 2020

Superseeded by #358

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

Successfully merging this pull request may close these issues.

3 participants