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

[Brainstorming] Isolate plugin JavaScript in anonymous functions #2200

Closed
astateofblank opened this Issue Nov 2, 2017 · 27 comments

Comments

7 participants
@astateofblank
Contributor

astateofblank commented Nov 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@foosel

foosel Nov 3, 2017

Owner

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 :)

Owner

foosel commented Nov 3, 2017

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

This comment has been minimized.

Show comment
Hide comment
@BillyBlaze

BillyBlaze Nov 3, 2017

Collaborator

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)

Collaborator

BillyBlaze commented Nov 3, 2017

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

This comment has been minimized.

Show comment
Hide comment
@foosel

foosel Nov 6, 2017

Owner

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).

Owner

foosel commented Nov 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@malnvenshorn

malnvenshorn Nov 6, 2017

Contributor

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() {...}).

Contributor

malnvenshorn commented Nov 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@foosel

foosel Nov 27, 2017

Owner

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?

Owner

foosel commented Nov 27, 2017

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?

@foosel foosel removed the status:solved label Nov 27, 2017

@BillyBlaze

This comment has been minimized.

Show comment
Hide comment
@BillyBlaze

BillyBlaze Nov 27, 2017

Collaborator

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.

Collaborator

BillyBlaze commented Nov 27, 2017

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

This comment has been minimized.

Show comment
Hide comment
@foosel

foosel Nov 27, 2017

Owner

What I'm seeing in TouchUI is this:

image

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.

Owner

foosel commented Nov 27, 2017

What I'm seeing in TouchUI is this:

image

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

This comment has been minimized.

Show comment
Hide comment
@BillyBlaze

BillyBlaze Nov 27, 2017

Collaborator

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?

Collaborator

BillyBlaze commented Nov 27, 2017

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

This comment has been minimized.

Show comment
Hide comment
@malnvenshorn

malnvenshorn Nov 27, 2017

Contributor

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.

Contributor

malnvenshorn commented Nov 27, 2017

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

This comment has been minimized.

Show comment
Hide comment
@BillyBlaze

BillyBlaze Nov 27, 2017

Collaborator

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.

Collaborator

BillyBlaze commented Nov 27, 2017

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 added a commit that referenced this issue Nov 27, 2017

@foosel

This comment has been minimized.

Show comment
Hide comment
@foosel

foosel Nov 27, 2017

Owner

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".

Owner

foosel commented Nov 27, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ntoff

ntoff Nov 28, 2017

Contributor

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.

Contributor

ntoff commented Nov 28, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jneilliii

jneilliii Nov 28, 2017

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.

jneilliii commented Nov 28, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ntoff

ntoff Nov 29, 2017

Contributor

@jneilliii I'm testing against the same commit this person is using #2246

ad246de

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.

Contributor

ntoff commented Nov 29, 2017

@jneilliii I'm testing against the same commit this person is using #2246

ad246de

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

This comment has been minimized.

Show comment
Hide comment
@foosel

foosel Nov 29, 2017

Owner

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.

Owner

foosel commented Nov 29, 2017

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 added a commit that referenced this issue Nov 29, 2017

Feature flag to enable legacy plugin assets
Work around issues like #2246 until the plugins that are affected are
fixed.

To be removed in 1.3.8.

See also #2200

foosel added a commit that referenced this issue Nov 29, 2017

Wrap all plugin's JS assets into same IIFE
As discussed in #2200.

For this to work we've introduced sub-bundles per plugin for the JS
assets. CSS and LESS on the other hand are basically handled as before.
We use a custom bundle class here since it's apparently not possible to
keep a sub bundle from inheriting the parent's filters, thus not
allowing us something like

  packed_plugins [js_plugin_delimiter_bundler]
    packed_plugin_a [js_delimiter_bundler]
    packed_plugin_b [js_delimiter_bundler]
    ...

The individiual bundles per plugin would inherit
js_plugin_delimiter_bundler and we'd get a double wrapper - not helpful.

Instead we now have a modified JsPluginBundle that overwrites
Bundle._merge_and_apply to further wrap the returned hunk.
@foosel

This comment has been minimized.

Show comment
Hide comment
@foosel

foosel Nov 29, 2017

Owner

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

We now have a new feature flag:

image

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.

Owner

foosel commented Nov 29, 2017

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

We now have a new feature flag:

image

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

This comment has been minimized.

Show comment
Hide comment
@BillyBlaze

BillyBlaze Nov 29, 2017

Collaborator

That looks amazing, again 👍 Thanks!

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

Collaborator

BillyBlaze commented Nov 29, 2017

That looks amazing, again 👍 Thanks!

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

@foosel

This comment has been minimized.

Show comment
Hide comment
@foosel

foosel Nov 30, 2017

Owner

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.

Owner

foosel commented Nov 30, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ntoff

ntoff Nov 30, 2017

Contributor

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?

Contributor

ntoff commented Nov 30, 2017

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

This comment has been minimized.

Show comment
Hide comment
@malnvenshorn

malnvenshorn Nov 30, 2017

Contributor

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.

Contributor

malnvenshorn commented Nov 30, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ntoff

ntoff Nov 30, 2017

Contributor

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

Contributor

ntoff commented Nov 30, 2017

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

@BillyBlaze

This comment has been minimized.

Show comment
Hide comment
@BillyBlaze

BillyBlaze Nov 30, 2017

Collaborator

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.

Collaborator

BillyBlaze commented Nov 30, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jneilliii

jneilliii Nov 30, 2017

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

jneilliii commented Nov 30, 2017

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

@foosel

This comment has been minimized.

Show comment
Hide comment
@foosel

foosel Dec 1, 2017

Owner

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.

Owner

foosel commented Dec 1, 2017

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

This comment has been minimized.

Show comment
Hide comment
@BillyBlaze

BillyBlaze Dec 1, 2017

Collaborator

@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.

Collaborator

BillyBlaze commented Dec 1, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@ntoff

ntoff Dec 1, 2017

Contributor

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.

Contributor

ntoff commented Dec 1, 2017

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

This comment has been minimized.

Show comment
Hide comment
@foosel

foosel Dec 12, 2017

Owner

Closing because 1.3.6 was just released.

Owner

foosel commented Dec 12, 2017

Closing because 1.3.6 was just released.

@foosel foosel closed this Dec 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment