Octoprint: [Brainstorming] Isolate plugin JavaScript in anonymous functions

[astateofblank]

Apologies if this isn't the correct format - I chose [Brainstorming] since this a bit of a finger pointing issue that OctoPrint can address systematically, and there are solutions I'm sure I'm not considering.

By default, OctoPrint is bundling plugins' static JavaScript assets with Flask Assets (webassets). A plugin can declare 'use strict'; globally (OctoPrint Anywhere currently does) that all other plugins then have to adhere to. A missed var declaration or global var use in another plugin could then cause errors with no clear responsibility.

My suggestion is to attempt to use Flask Assets filters to wrap each file being bundled in an anonymous function. 'use strict'; declarations would then be scoped to specific functions, and other conflicts would likely be avoided. If I get validation that this is acceptable and/or feasible I may be able to invest the time in a pull request.

The plugin template does have everything wrapped in a top level function, but author preference (for linting, etc) will lead to variations. Additionally, not all plugins (ie, OctoPrint Anywhere) require jQuery. Given that non-compliant plugins can be approved and added to the repository, I think this should be enforced by the build system if possible.

An alternative solution for this particular issue would be to enforce 'use strict'; for all plugins, but that still leaves the responsibility with each plugin author. Individually wrapping the plugins seems like the most elegant solution.

If it stands that OctoPrint's plugin ecosystem leaves the compatibility burdens on developers, it is the end users (yo!) that will experience problems. Global try/catch blocks might be an additional improvement to prevent plugins' JavaScript from causing compatibility issues.

[foosel]

Anonymous function plus try-catch-block makes a ton of sense 👍

It can even be easily limited to only third party plugin assets since those are processed into their own bundle.

Happy to receive a pull request to get this into maintenance/1.3.6.dev sooner rather than later :)

[BillyBlaze]

Good find! I had exactly the same issue in my plugin with use strict and a global variable. (e.g. I forgot a var too) and didn't think of this situation.

Enforcing 'use strict'; can be a deal breaker for some plugins. So I would propose to do this in 1.4.0 (givin' some time for developers to fix their stuff)

[foosel]

Just merged the PR, so the wrap in an anonymous function plus some try-catch-block around it will be part of 1.3.6. Thanks! :)

---

About enforcing 'use strict'; down the road, I'm not sure how best to go about that. I fear it would cause a LOT of issues for devs that don't breathe JS daily (myself btw possibly included 😆) that nevertheless could add nice functionality through their plugins but might then be discouraged. So while in general I do agree that it would be better to enforce strict mode, I'm not entirely sure how to best go for it without potentially frustrating excited new devs (not that there aren't other things that have a potential to do so, just not sure if adding yet another one is a great idea).

[malnvenshorn]

Generally it's a good thing to have the strict mode enabled but there is a reason why it is not used by default, it's not backward compatible. If you use third party libraries which are not written with this in mind you are running into issues. Because of this I wouldn't enforce _"use_strict";_. But, I think, nothing would speak against adding this to the plugin template inside $(function() {...}).

[foosel]

I'll sadly have to revert this change for now. Problem is that there are a bunch of plugins relying on being able to define globals (e.g. TouchUI and Fullscreen by @BillyBlaze, NavbarTemp by @imrahil) that break with this adjustment in more or less serious ways. See for example #2246.

The question is how to solve this now so that long term we have plugins run more isolated and hence less likely to break things in unexpected ways but still stay backwards compatible (or at the very least create some adjustment period).

Any ideas?

[BillyBlaze]

The fullscreen and navbar would be easily fixed by making it: window.myFunction = ... instead of function myFunction() {}. Heck I could even just play nice there, and use it in the viewmodel.

As for TouchUI, I can't see without debugging what exactly is going wrong there to see what impact this has.

[foosel]

What I'm seeing in TouchUI is this:

I figure that could also be easily solved through a simple window.TouchUI = TouchUI at the end of touchui.bundled.js.

What I'm worried here is less how to fix individual plugins, but rather that as things are currently, updating to 1.3.6 will/would make an unknown number of plugins potentially non-functional. Which is a bad thing for users, and also for my stress levels after release ;)

I'm unsure how to solve that. Just removing the anonymous function again would of course fix this issue, but on the other hand produce other problems with e.g. use strict, which was the very reason this wrapper was introduced in the first place.

[BillyBlaze]

Thanks, that makes sense! And looks easily fixed.

I think it would be a major improvement to add IIFE's. As this would also make TouchUI more stable with 3rd party plugins.

Could we introduce this perhaps in 1.4.0?

[malnvenshorn]

I totally understand your feelings about this change breaking plugins. But I think this should be merged rather sooner than later, because it will affect only more plugins and not less. You could make an announcement in the OctoPrint blog with a specific time frame in mind so every developer can check their plugins against this change. I wouldn't wait till the release of 1.4, because at the moment it is very easy to break other plugins and it is very hard for a developer to narrow down such issues.

[BillyBlaze]

After a brief discussion on IRC, we agree that this should land as soon as possible.

I have made a suggestion to wrap IIFE around all the files that are bundled inside a plugin rather then making an IIFE around each file. This would make backwards-compatibility with older plugins better. (i.e. TouchUI wouldn't generate such errors as above, since it's scoped inside the same function)

Both the plugins Navbar and Fullscreen would require some attention. But I am more then willing to adjust the fullscreen and to make this play nice and be applied through knockout rather then attaching it to the window.

Beside attaching stuff to the window also feels dirty.

[foosel]

Just for the record, I've disable use of the "offending" filter on maintenance and devel for now, until I manage to pound webassets into shape so that it allows me to do the above. I was hoping I'd be able to finalize that today, but now it might have to wait until Friday. So... temporary "fix".

[ntoff]

What I'm worried here is less how to fix individual plugins, but rather that as things are currently, updating to 1.3.6 will/would make an unknown number of plugins potentially non-functional. Which is a bad thing for users, and also for my stress levels after release ;)

I have no clue what I'm doing so probably all of my plugins will break, but if the checks are disabled, how do I know what I need to fix? Is there a branch with the checks still enabled? I looked at the branches but there wasn't an obvious "improve/anonymizeplugins" branch or anything.

[jneilliii]

I'm afraid of the same thing. I'm sure mine will break too as I'm not a professionally trained/taught programmer. This was made apparent when my plugin brought octoprint to its knees and spawned this change I'm sure.

[ntoff]

@jneilliii I'm testing against the same commit this person is using https://github.com/foosel/OctoPrint/issues/2246

https://github.com/foosel/OctoPrint/commit/ad246deb9e7f587b75ce1a0eb6ba6f52900c2029

Also I noticed the plugins only seem to break if you have assets bundled, I like to turn that off because it makes things easier but apparently it also stops some issues from showing up. So now I'm scared all my plugins that seem to work, will probably break once asset bundling is turned on.

[foosel]

First of all - you are most likely fine. The issue only arises if you declare globals in your javascript that you then rely on to be available outside of your own JS files. E.g. you define a helper function to format some kind of value and then try to use that directly in some template. For example, if you have this as your JS file:

$(function() {
    function YourViewModel(parameters) {
        var self = this;
        self.value = ko.observable("value");
});

function myHelper(value) {
    return "formattedValue: " + value;
}

and then this as part of one of your templates:

<span data-bind="text: myHelper($data.value)"></span>

then you would run into issues, since the myHelper in your template would no longer resolve. If you are only using regular run-off-the-mill view model mechanisms (as shown e.g. in the plugin tutorial and present in the bundled plugins) you won't run into any trouble.

The plugins that we so far know of having issues with this, TouchUI, Fullscreen and NavbarTemp, (currently) rely on such globals in the one or other way. TouchUI has multiple assets and relies on the presence of a global defined in the one to be accessible in another. Fullscreen and NavbarTemp both declare a global helper function like in the example above and use it in a data binding, leading to problems.

In all those cases, explicitly declaring the function/variable/whatnot that's supposed to be global instead of relying on the JS default (dumping everything into the global namespace unless you say otherwise) will solve the issue in a backwards compatible matter. To go back to our example from above, this will make it work again, even with the new approach:

$(function() {
    function YourViewModel(parameters) {
        var self = this;
        self.value = ko.observable("value");
});

window.myHelper = function(value) {
    return "formattedValue: " + value;
}

Also I noticed the plugins only seem to break if you have assets bundled

Yes, because the bundling is what we're talking about modifying here/had modified/will modify again. So far, when bundling, OctoPrint instructed webassets to bundle plugin JS files like this:

// contents of fileA.js

;

// contents of fileB.js

;

// and so on

That has the problem that if fileB.js declares 'use strict';, anything following after that will also be forced into strict more - which doesn't work and breaks things horribly if the following files aren't written to conform to strict mode. With the change, that gets switched to:

(function() {
    try {
        // contents of fileA.js
    } catch (error) {
        log.error("Error in bundled asset fileA.js:", (error.stack || error));
    }
})();
(function() {
    try {
        // contents of fileB.js
    } catch (error) {
        log.error("Error in bundled asset fileB.js:", (error.stack || error));
    }
})();
// and so on

Basically, the contents of the bundled assets are moved into their own anonymous functions that get then immediately executed, preventing stuff like 'use strict'; from affecting other files, and some additional error handling also finds some other potential issues that might have caused trouble before. The downside (or actually, if you ask me, the upside) is that foo = "bar" no longer makes foo global by default.

---

With that being said, it currently looks like getting webassets (which is used for the bundling) to apply a filter per "sub bundle" is not possible out of the box, so I'm not sure yet if I'll be able to get the idea implemented that we discussed on IRC and that's outlined above by @BillyBlaze. I'll give it another go today, but I might have to resort to introducing a feature flag instead - shipping that defaulting to on and such allowing users who rely on plugins that haven't yet been updated to switch back to the old behaviour until those plugins are updated, with a defined cutoff date (say, in three to six months) where that flag will be removed again and "on" hardcoded.

[foosel]

So, good news every one. I've done both.

We now have a new feature flag:

Defaults to off (as seen here) which wraps plugin JS assets into IIFEs (I learnt a new word thanks to @BillyBlaze ;)). And since I finally wrangled webassets into submission earlier today, we have one IIFE per plugin. Looks somewhat like this now:

// JS assets for plugin touchui
(function () {
    try {
        // source: plugin/touchui/js/touchui.libraries.js
        // [contents...]
        ;

        // source: plugin/touchui/js/touchui.bundled.js
        // [contents...]
        ;

        // source: plugin/touchui/js/touchui.bootstrap.js
        // [contents...]
        ;

    } catch (error) {
        log.error("Error in JS assets for plugin touchui:", (error.stack || error));
    }
})();           

The plan now is to inform about this in a blog post (to be written) so that users and authors know what to expect, then link that from the above feature flag help, the RC release notes & announcements and of course the final stable release notes & announcement.

[BillyBlaze]

That looks amazing, again 👍 Thanks!

However since we now have a toggle, How about introducing "Use strict;" too?

[foosel]

How about introducing "Use strict;" too?

I know where you are coming from, but I still have a bad feeling about this. The current change in how we handle things with the introduction of the IIFEs should be somewhat tame with regards to side effects on existing plugins. There will be the one or other plugin that runs into trouble due to the global declaration stuff, but that can be easily solved (in a somewhat meh kinda way).

But if we now enforce strict mode, we open a whole different can of worms that will probably break a LOT of stuff, also in case where libraries are bundled. The flag I now added is planned to be removed in 1.3.8 again - that's short enough that it puts a bit of pressure on authors to adjust things, and the work involved to adjust things is little enough that it should hopefully suffice for even the most busy of plugin maintainers. I don't see this with strict mode.

Don't get me wrong - after finally reading up on it personally I'm all for using strict mode for plugin development (and it's something I really need to get on with as well). But I don't think that forcing its use on everyone as part of a point release is a good idea. It's bad enough that we are introducing this change now that's basically breaking backwards compatibility, and the only reason why I feel semi comfortable with that is a) the flag that helps users and b) the arguments above that the current state is causing a major debugging headache for authors.

How about adding another flag that allows enabling strict mode. Leave this off as default for now. Switch to on by default with 1.4.0. Remove then at a later version? That would allow authors to test their stuff against strict mode, but not break things left and right at first.

[ntoff]

Don't get me wrong - after finally reading up on it personally I'm all for using strict mode for plugin development

Maybe just add 'use strict'; to the plugin template? To at least force anyone making new plugins in the future to at least try it?

It's bad enough that we are introducing this change now that's basically breaking backwards compatibility, and the only reason why I feel semi comfortable with that is a) the flag that helps users and b) the arguments above that the current state is causing a major debugging headache for authors.

Well if it prevents rogue plugins from breaking A) octoprint and B) other plugins, maybe it's worth breaking backwards compatibility for the sake of keeping users safer / making it easier to uninstall rogue plugins without having to log in via ssh.

How about adding another flag that allows enabling strict mode. Leave this off as default for now. Switch to on by default with 1.4.0. Remove then at a later version? That would allow authors to test their stuff against strict mode, but not break things left and right at first.

I guess just make it one of the "hidden" developer flags (the ones that don't have any entry in the regular settings menu). Then you won't get random users turning it on wondering what it's for.

But if we're talking about developers here, I'm as dumb as a post but even I'm capable of putting 'use strict'; at the start of my javascript, is that not enough to test against strict mode?

[malnvenshorn]

But if we're talking about developers here, I'm as dumb as a post but even I'm capable of putting 'use strict'; at the start of my javascript, is that not enough to test against strict mode?

The 'use strict'; needs to be before every other statement to take affect. Which means also before the try-catch-block. Therefore if you want to add it to your script you need to wrap your whole script in another function or apply the rule to each function separately.

[ntoff]

ok got it, I did mention I'm as dumb as a post :)

[BillyBlaze]

After another brief discussion on IRC, we decided to make a small crawler that would validate all the JS files within all the registered OctoPrint plugins.

After a few lines, setting up a config file for ESLINT and after declaring all of the OctoPrint globals I managed to produce an outcome.

Sadly, it's not a positive outcome, to many plugins will be affected by use strict;, and those even include popular plugins. Since a new release is scheduled we decided to post-pone introducing use strict.

However I did propose, to introduce use strict in 1.4.0 with the possibility to disable strict mode. This can be disabled only within the plugin for that specific plugin.

[jneilliii]

Can you publish the results of your crawl so us "developers" know they need to fix stuff?

[foosel]

I've published a post about this on the OctoBlog.

@jneilliii, @ntoff just to make you relax a bit, I just did a quick look through the JS assets that are part of those plugins you have published on the repo and I didn't immediately see anything that should cause issues with this IIFE change we introduced here.

[BillyBlaze]

@jneilliii; I am very reluctant to publish these results, I don't want to hit someone nerve and make this bigger then it is, besides that it might create an impression of who is right or wrong. Which is absolutely not the case.

However I can lookup the results for your plugins:

https://github.com/jneilliii/OctoPrint-Tasmota/blob/master/octoprint_tasmota/static/js/tasmota.js#L69
plug = ... should be var plug = ...

https://github.com/jneilliii/OctoPrint-TPLinkSmartplug/blob/master/octoprint_tplinksmartplug/static/js/tplinksmartplug.js#L68
plug = ... should be var plug = ...

STLViewer:
There are some undefined variables inside the library you're using within jsc3d.js:
3642:13 | Error | 'minY' is not defined. | no-undef
3642:20 | Error | 'minZ' is not defined. | no-undef
3643:13 | Error | 'maxY' is not defined. | no-undef
3643:20 | Error | 'maxZ' is not defined. | no-undef
3655:10 | Error | 'minY' is not defined. | no-undef
3656:4 | Error | 'minY' is not defined. | no-undef
3657:10 | Error | 'maxY' is not defined. | no-undef
3658:4 | Error | 'maxY' is not defined. | no-undef
3659:10 | Error | 'minZ' is not defined. | no-undef
3660:4 | Error | 'minZ' is not defined. | no-undef
3661:10 | Error | 'maxZ' is not defined. | no-undef
3662:4 | Error | 'maxZ' is not defined. | no-undef
3666:19 | Error | 'minY' is not defined. | no-undef
3667:19 | Error | 'minZ' is not defined. | no-undef
3669:19 | Error | 'maxY' is not defined. | no-undef
3670:19 | Error | 'maxZ' is not defined. | no-undef
5251:21 | Error | 'VBArray' is not defined. | no-undef

BLTouch:
No errors.

[ntoff]

just to make you relax a bit

I'm pretty sure one of my registered plugins has no javascript (m84 rewrite) and all the cyborg theme does is detect touchui and turn off, which might as well be removed from the repo in the future anyway thanks to themeify. So the registered ones weren't my source of concern.

It's the non registered ones that scare me. The ones I made "just for me" but stupidly put on github A) to make it easier to install on my printers and sync across my windows / linux computers and B) because open source all the things! The ones that maybe aren't finished yet but do still work (and that I do use on my actual printers so I know they're not too catastrophic). I know the "settings" branch of my tab icons plugin breaks octoprint (dunno how to fix that), and the fan speed slider plugin which you know was using global vars, because you told me not to :p Though I'm pretty sure the fan speed control plugin is done now and should be safe, I think. Which is why I'd like a "strict af" branch of octoprint to test against because I know I'm not the only one using these semi-finished plugins.

[foosel]

Closing because 1.3.6 was just released.