Better friendly error help for using p5 globals in top-level code. #1256

Merged
merged 1 commit into from Feb 23, 2016

Conversation

Projects
None yet
2 participants
@toolness
Member

toolness commented Feb 13, 2016

This fixes #1247 by doing the following:

  • Adding everything on p5.prototype (including constants like PI) on to the list of symbol names to search for in thrown error messages during page load. This should help with issues like #1237 and #903, and it will also help with some code that uses p5 addons too, e.g. using p5.dom's select() in top-level code. This might result in more false positives than are helpful, but maybe we should wait for bug reports to come in before deciding that?
  • In the friendly error message, add a link to the helpful FAQ question Why can't I assign variables using p5 functions and variables before setup()? so users can get more details.
  • Using more concise language to help the user, e.g.:
    • Did you just try to use p5.js's HALF_PI constant?
    • Did you just try to use p5.js's random() function?
    • Did you just try to use p5.js's mouseX variable?

Since this functionality is no longer really trivial, I also added a simple unit test suite.

I also added a bunch of comments in the code to explain some of the implementation decisions, as well as potential future workarounds.

+ // At present, p5 only adds its constants to p5.prototype during
+ // construction, which may not have happened at the time a
+ // ReferenceError is thrown, so we'll manually add them to our list.
+ getSymbols(require('./constants'))

This comment has been minimized.

@toolness

toolness Feb 13, 2016

Member

I was actually kind of surprised that the constants like PI aren't added to p5.prototype until p5's constructor. Was this intentional?

It's particularly odd to me because the comment in core.js says // attach constants to p5 instance, yet the code is actually adding the constants to p5.prototype, which is part of the class, rather than the instance. I could file a separate bug for this if needed.

Update: filed #1312.

@toolness

toolness Feb 13, 2016

Member

I was actually kind of surprised that the constants like PI aren't added to p5.prototype until p5's constructor. Was this intentional?

It's particularly odd to me because the comment in core.js says // attach constants to p5 instance, yet the code is actually adding the constants to p5.prototype, which is part of the class, rather than the instance. I could file a separate bug for this if needed.

Update: filed #1312.

This comment has been minimized.

@lmccart

lmccart Feb 23, 2016

Member

I don't think there is a real reason for this, I have a feeling it's legacy code that hasn't been properly cleaned up. Please feel free to open an issue for this.

@lmccart

lmccart Feb 23, 2016

Member

I don't think there is a real reason for this, I have a feeling it's legacy code that hasn't been properly cleaned up. Please feel free to open an issue for this.

@@ -38,6 +38,7 @@
<script src="unit/core/core.js" type="text/javascript" ></script>
<!--<script src="unit/core/2d_primitives.js" type="text/javascript" ></script>-->
<script src="unit/core/curves.js" type="text/javascript" ></script>
+ <script src="unit/core/error_helpers.js" type="text/javascript" ></script>

This comment has been minimized.

@toolness

toolness Feb 13, 2016

Member

Note that I'm not adding this suite to test-minified.html because the friendly error help isn't supposed to be part of minified (optimized) builds.

@toolness

toolness Feb 13, 2016

Member

Note that I'm not adding this suite to test-minified.html because the friendly error help isn't supposed to be part of minified (optimized) builds.

@toolness toolness referenced this pull request in toolness/p5.js-docker Feb 13, 2016

Closed

Provide a way to run p5.js unit tests #8

lmccart pushed a commit that referenced this pull request Feb 23, 2016

Lauren McCarthy
Merge pull request #1256 from toolness/better-global-friendly-errors
Better friendly error help for using p5 globals in top-level code.

@lmccart lmccart merged commit 4136068 into processing:master Feb 23, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lmccart

This comment has been minimized.

Show comment
Hide comment
@lmccart

lmccart Feb 23, 2016

Member

this is great!

Member

lmccart commented Feb 23, 2016

this is great!

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