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

Log helpful warnings if user overwrites p5 global functions. #1318

Merged
merged 4 commits into from Mar 28, 2016

Conversation

Projects
None yet
2 participants
@toolness
Member

toolness commented Mar 23, 2016

This is an attempt to fix #1317 and prevent future occurrences of #1314 by allowing the overwriting of p5 globals, but logging helpful warning messages informing the user of what just happened.

Example 1

var text = "blarg";

function setup() {
}

This will cause the following message to be logged to the console:

p5 had problems creating the global function "text", possibly because your
code is already using that name as a variable. You may want to rename
your variable to something else.

Example 2

function setup() {
  text += "blarg";
}

This will cause the following message to be logged to the console:

You just changed the value of "text", which was a p5 function. This could cause
problems later if you're not careful.

Limitations

  • Modifying a global p5 function via delete or Object.defineProperty() won't log any warnings, but it's unlikely that users--especially novices who could really use the assistance--will be using these.
  • Warnings are only logged for p5 global functions, so the user won't see anything if e.g. their code conflicts with mouseX or mouseIsPressed. We might want to tackle those in a separate PR if we want to address them, since I think the implementation will be a bit different and independent of the changes in this PR.
  • There's a potential performance hit from the fact that this mechanism adds the overhead of a function call every time a global p5 function is accessed. It'd be nice if we could add a build-time conditional that only adds this extra functionality for non-minified/non-optimized builds, as per the friendly error system. I know how to do this kind of thing with webpack but not browserify. Update: this is actually a feature of UglifyJS, which I mention in #1318 (comment).
  • This PR doesn't currently have any unit tests. I'd like to add some! We have unit tests now!

@toolness toolness changed the title from [WIP] Log helpful warnings if user overwrites p5 globals. to [WIP] Log helpful warnings if user overwrites p5 global functions. Mar 23, 2016

@toolness toolness changed the title from [WIP] Log helpful warnings if user overwrites p5 global functions. to Log helpful warnings if user overwrites p5 global functions. Mar 24, 2016

@toolness

This comment has been minimized.

Show comment
Hide comment
@toolness

toolness Mar 24, 2016

Member

Ok @lmccart, I think this PR is ready for review!

Member

toolness commented Mar 24, 2016

Ok @lmccart, I think this PR is ready for review!

// https://github.com/processing/p5.js/issues/1317
if (prop in globalObject && !(prop in propsToForciblyOverwrite)) {
throw new Error('global "' + prop + '" already exists');

This comment has been minimized.

@lmccart

lmccart Mar 27, 2016

Member

Can you explain a little more about what's happening here? It looks like the log call on line 587 will display the console.log warning, which is the behavior we want (as noted in line 563). So in which case would an error get thrown?

@lmccart

lmccart Mar 27, 2016

Member

Can you explain a little more about what's happening here? It looks like the log call on line 587 will display the console.log warning, which is the behavior we want (as noted in line 563). So in which case would an error get thrown?

This comment has been minimized.

@toolness

toolness Mar 27, 2016

Member

Oh, good question... I actually ran into this problem before adding the explicit throw in lines 570-571, when I used the following sketch:

var text;

function setup() {}

Basically browsers threw an actual error when I was trying to call Object.defineProperty, claiming that I was trying to "redefine a non-configurable property". So I originally added the try/catch to detect for that, but I ended up later adding lines 570-571, which check for such a condition for us, so we might be able to get rid of the try/catch...

That said, because this functionality will always be called in global mode, and because it's sort of "nice to have" functionality rather than "must have" functionality, I thought it might be a good idea to leave the try/catch with the fallback to legacy behavior in there just in case there are really weird edge cases that we don't know about... I can remove it if you want though!

@toolness

toolness Mar 27, 2016

Member

Oh, good question... I actually ran into this problem before adding the explicit throw in lines 570-571, when I used the following sketch:

var text;

function setup() {}

Basically browsers threw an actual error when I was trying to call Object.defineProperty, claiming that I was trying to "redefine a non-configurable property". So I originally added the try/catch to detect for that, but I ended up later adding lines 570-571, which check for such a condition for us, so we might be able to get rid of the try/catch...

That said, because this functionality will always be called in global mode, and because it's sort of "nice to have" functionality rather than "must have" functionality, I thought it might be a good idea to leave the try/catch with the fallback to legacy behavior in there just in case there are really weird edge cases that we don't know about... I can remove it if you want though!

This comment has been minimized.

@lmccart

lmccart Mar 27, 2016

Member

Makes sense. Would you mind just adding a little line note here so we remember what's going on and this doesn't cause confusion for others?

@lmccart

lmccart Mar 27, 2016

Member

Makes sense. Would you mind just adding a little line note here so we remember what's going on and this doesn't cause confusion for others?

@lmccart

This comment has been minimized.

Show comment
Hide comment
@lmccart

lmccart Mar 27, 2016

Member

This looks great, one question inline.

Limitations 1 and 2 sound find for now, your notes make sense.

Regarding the potential performance hit... is it possible to do something similar like we were doing with the original friendly errors, where at build time in the minified version only, the argument checker function was overwritten so it wouldn't slow things down.

I am imagining something like overwriting p5.prototype._createFriendlyGlobalFunctionBinder, though I guess we'd need to change/remove the assignment on line 445.

Member

lmccart commented Mar 27, 2016

This looks great, one question inline.

Limitations 1 and 2 sound find for now, your notes make sense.

Regarding the potential performance hit... is it possible to do something similar like we were doing with the original friendly errors, where at build time in the minified version only, the argument checker function was overwritten so it wouldn't slow things down.

I am imagining something like overwriting p5.prototype._createFriendlyGlobalFunctionBinder, though I guess we'd need to change/remove the assignment on line 445.

@toolness

This comment has been minimized.

Show comment
Hide comment
@toolness

toolness Mar 27, 2016

Member

Yep, overwriting _createFriendlyGlobalFunctionBinder is definitely one way to do it...

Do you know if p5's build system uses UglifyJS under the hood? If it does, we can actually use uglify's Global Definitions functionality to conditionally define _createFriendlyGlobalFunctionBinder at build-time. This way the "friendly" version of the binder won't even take up extra space in the minified JS file!

Member

toolness commented Mar 27, 2016

Yep, overwriting _createFriendlyGlobalFunctionBinder is definitely one way to do it...

Do you know if p5's build system uses UglifyJS under the hood? If it does, we can actually use uglify's Global Definitions functionality to conditionally define _createFriendlyGlobalFunctionBinder at build-time. This way the "friendly" version of the binder won't even take up extra space in the minified JS file!

@lmccart

This comment has been minimized.

Show comment
Hide comment
@lmccart

lmccart Mar 27, 2016

Member

indeed we are using uglify, that's how it worked in the previous version of friendly errors as well, I believe:
https://github.com/processing/p5.js/blob/master/Gruntfile.js#L253

Member

lmccart commented Mar 27, 2016

indeed we are using uglify, that's how it worked in the previous version of friendly errors as well, I believe:
https://github.com/processing/p5.js/blob/master/Gruntfile.js#L253

@toolness

This comment has been minimized.

Show comment
Hide comment
@toolness

toolness Mar 27, 2016

Member

Oh cool! Sure, I will add the conditional compilation bit and the comment.

Member

toolness commented Mar 27, 2016

Oh cool! Sure, I will add the conditional compilation bit and the comment.

@toolness

This comment has been minimized.

Show comment
Hide comment
@toolness

toolness Mar 28, 2016

Member

Ok, in 365cc78 I added a commit that only includes this fancy behavior in un-minified builds. Unlike with _validateParameters and _friendlyFileLoadError, however, this is not done by overwriting the functions to be no-ops in the uglify footer options.

Instead, it's done by adding an uglifyjs global_defs option that sets a global called IS_MINIFIED for minified builds. The implementation of _createFriendlyGlobalFunctionBinder checks for the existence of IS_MINIFIED in its implementation. The nice thing about this is that because uglifyjs knows that IS_MINIFIED has a constant value, it uses its dead code removal functionality to excise the unused code from the minified version entirely. This can be evidenced by running grep 'not careful' lib/p5.min.js--the string "not careful" is part of one of our warning messages--and verifying that no matches are found.

I recommend using this kind of feature for _validateParameters and _friendlyFileLoadError, too, since it will reduce the size of the minified build by eliminating the whole friendly debugger from it (which will be increasingly useful as the friendly debugger's functionality grows). I also like the approach because it makes it more obvious to readers that the functionality won't be available in minified builds. If you agree, I can file a bug for it.

Note: I originally wanted the variable to be called DEBUG instead of IS_MINIFIED, but one of the challenges of this was defaulting DEBUG to true; because p5's build system first generates p5.js and uses that as the source for p5.min.js, doing this proved difficult, so I opted to call the variable IS_MINIFIED and only define it in minified builds.

Member

toolness commented Mar 28, 2016

Ok, in 365cc78 I added a commit that only includes this fancy behavior in un-minified builds. Unlike with _validateParameters and _friendlyFileLoadError, however, this is not done by overwriting the functions to be no-ops in the uglify footer options.

Instead, it's done by adding an uglifyjs global_defs option that sets a global called IS_MINIFIED for minified builds. The implementation of _createFriendlyGlobalFunctionBinder checks for the existence of IS_MINIFIED in its implementation. The nice thing about this is that because uglifyjs knows that IS_MINIFIED has a constant value, it uses its dead code removal functionality to excise the unused code from the minified version entirely. This can be evidenced by running grep 'not careful' lib/p5.min.js--the string "not careful" is part of one of our warning messages--and verifying that no matches are found.

I recommend using this kind of feature for _validateParameters and _friendlyFileLoadError, too, since it will reduce the size of the minified build by eliminating the whole friendly debugger from it (which will be increasingly useful as the friendly debugger's functionality grows). I also like the approach because it makes it more obvious to readers that the functionality won't be available in minified builds. If you agree, I can file a bug for it.

Note: I originally wanted the variable to be called DEBUG instead of IS_MINIFIED, but one of the challenges of this was defaulting DEBUG to true; because p5's build system first generates p5.js and uses that as the source for p5.min.js, doing this proved difficult, so I opted to call the variable IS_MINIFIED and only define it in minified builds.

@lmccart

This comment has been minimized.

Show comment
Hide comment
@lmccart

lmccart Mar 28, 2016

Member

awesome! yes, I agree this approach would be preferable for _validateParameters and _friendlyFileLoadError, as well. this all looks great, going ahead with the merge.

Member

lmccart commented Mar 28, 2016

awesome! yes, I agree this approach would be preferable for _validateParameters and _friendlyFileLoadError, as well. this all looks great, going ahead with the merge.

@lmccart lmccart merged commit 946666a into processing:master Mar 28, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment