more class to es6

[asukaminato0721]

Resolves #3758

Changes:

More class to es6, this is a continue to #6075

Screenshots of the change:

PR Checklist

• npm run lint passes
• Inline documentation is included / updated
• Unit tests are included / updated

Now the test throws 3 errors, but the npm run build result seems to work properly.

  1780 passing (34s)
  10 pending
  3 failing
  1) Error Helpers
       validateParameters: argument tree
         arg tree is built properly:
     TypeError: Cannot read properties of undefined (reading 'seen')
      at Context.<anonymous> (unit/core/error_helpers.js:279:31)

  2) Global Error Handling
       identifies errors happenning internally:

      AssertionError: expected 2 to equal 1
      + expected - actual

      -2
      +1
      
      at assert.strictEqual (http://localhost:9001/node_modules/chai/chai.js:2329:32)
      at http://localhost:9001/test/unit/core/error_helpers.js:564:14

  3) Tests for p5.js sketch_reader
       detects reassignment of p5.js function (textSize from Typography) outside setup:

      AssertionError: expected 0 to equal 1
      + expected - actual

      -0
      +1
      
      at assert.strictEqual (http://localhost:9001/node_modules/chai/chai.js:2329:32)
      at http://localhost:9001/test/unit/core/error_helpers.js:987:16

[asukaminato0721]

I've dug into it, and found the problems are all in error_helper.js. the obj.seen. But since the rewrite is trivial. So I guess maybe there are some problems with the test.

[davepagurek]

You're probably right @wuyudi -- sorry I haven't had a chance to take a look just yet, but hopefully in the next day or so I'll get the time! then I can see if I can help updating either the test or the function the test is using.

[davepagurek]

Hey @wuyudi, I think the issue comes from creating classes like this:

p5.Color = class{ /* ... */ }

When parsing function arguments to deliver friendlier error messages, we check value.constructor.name. Since you're making an anonymous class, regardless of whether we assign it to a named value, its prototype's .name property will just be _class rather than Color. I think this is fixed if we use named classes, e.g.:

p5.Color = class Color { /* ... */ }

[asukaminato0721]

now there are only one

  10 pending
  1 failing
  1782 passing (30s)
  1) Global Error Handling
       identifies errors happenning internally:

      AssertionError: expected 2 to equal 1
      + expected - actual

      -2
      +1
      
      at assert.strictEqual (http://localhost:9001/node_modules/chai/chai.js:2329:32)
      at http://localhost:9001/test/unit/core/error_helpers.js:564:14

[davepagurek]

Does it still happen if you add a name for all the classes you've converted?

[asukaminato0721]

I add names for four classes lol, so "this" won't become a problem anymore.

[asukaminato0721]

it seems that the log is unrelated to the named class.

[davepagurek]

For this one, it looks like maybe it's from methods on the class still being defined by setting a function on the class prototype rather than using an ES6 class method. The check to see if an error is internal happens here:

p5.js/src/core/friendly_errors/fes_core.js

Line 507 in ab3e2ad

if (stacktrace[j].functionName.search('_main.default') !== -1) {

The test in question runs this code:

function setup() {
  let cnv = createCanvas(400, 400);
  cnv.mouseClicked(); // this is an error, a callback should be passed here
}

When the error is caught and it looks at the stack on v1.6.0, it looks like this:

After these changes, it looks like this:

It seems like this is partially a result of how Babel transforms classes, but I'm not 100% sure. @limzykenneth do you have any extra insights into what part might be adding the extra info to the name? I was looking at the options here https://babeljs.io/docs/babel-plugin-transform-classes but couldn't get anything to fix it.

Another option might be to find another way, other than the stack trace function name, to check if the error happened in an internal function or not. Maybe checking if the most recent item in the stack trace comes from a file called p5.js?

[limzykenneth]

So I did some digging, not gone all the way but might have something. While babel does indeed produce different code with the class syntax as opposed to the prototype syntax (class syntax essentially got transpiled into a sort of generator function while prototype mainly stay what they are), the main problem is that the messaged logged when an internal undefined parameter is encountered is different between 1.6.0 and this PR.

function setup() {
  let cnv = createCanvas(400, 400);
  cnv.mouseClicked(); // this is an error, a callback should be passed here
}

I don't know why this is not showing up in the test on the main branch when it shows up on built version of the library but basically, in 1.6.0 and in the test of the main branch, the above test code logs in the FES this message:

[
  '\n' +
    `🌸 p5.js says: [test.html, line 4] An error with message "Cannot read properties of undefined (reading 'bind')" occurred inside the p5js library when mouseClicked was called. If not stated otherwise, it might be an issue with the arguments passed to mouseClicked. (http://p5js.org/reference/#/p5/mouseClicked)`
]

which is only one message, whereas this PR and the built version of the main branch logs the following:

[
  '\n' +
    '🌸 p5.js says: \n' +
    '[p5.js, line 62480] Cannot read property of undefined. Check the line number in error and make sure the variable which is being operated is not undefined.\n' +
    '\n' +
    ' + More info: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Cant_access_property#what_went_wrong',
  '┌[http://localhost:9001/lib/p5.js:62480:29] \n' +
    '\t Error at line 62480 in Function._attachListener()\n' +
    '└[http://localhost:9001/lib/p5.js:62468:41] \n' +
    '\t Called from line 62468 in Function._adjustListener()\n' +
    '└[http://localhost:9001/lib/p5.js:62084:39] \n' +
    '\t Called from line 62084 in Renderer2D.mouseClicked()\n' +
    '└[http://localhost:9001/test/test.html:4:5] \n' +
    '\t Called from line 4 in setup()\n'
]

which are two messages. The actual intended message in this case should probably be just the one message (ie. the former).

This behaviour started in 012340e when p5.Element got converted to the class syntax and it made sense to show up here when changing the p5.Renderer syntax since it inherits p5.Element. I haven't been able to tell much more at this point as I don't fully understand FES, I'll defer to @almchung for that possibly. In any case we probably should double check p5.Element's implementation and also how it transpile through Babel.

A side note, I looked into letting Babel preserve the class syntax after transpilation but there are many areas in the library where the constructors are not called with the new keyword that causes test errors, unless those are fixed we can quite go this direction yet (most of them are actually coming from p5.sound).

[davepagurek]

there are many areas in the library where the constructors are not called with the new keyword that causes test errors, unless those are fixed we can quite go this direction yet (most of them are actually coming from p5.sound).

@limzykenneth do you have a way to get a list of those? If so, I can help convert them in the p5.sound repo (also I think the p5.sound build is fairly old and due for a rebuild, we can make a new build and see if it's still like that)

[limzykenneth]

@davepagurek They way I got those p5.Sound errors is by changing the browserlist key in package.json to the latest Chrome version (so that it definitely can handle class syntax) and run the build and test.

However, this will likely not be a viable way to solve this issue just yet because of the browsers we are supporting ("last 2 versions", "not dead"), the most problematic in that list is Opera Mini (and maybe some other even more obscure mobile browsers).

[davepagurek]

Ah ok makes sense!

So FES switches between those two messages based on whether it detects that the error happens within the library. Right now it does that on this line by checking the name of the function calls in the stacktrace to see if they include _main.default:

p5.js/src/core/friendly_errors/fes_core.js

Line 507 in ab3e2ad

if (stacktrace[j].functionName.search('_main.default') !== -1) {

I was looking into potential changes to the Babel settings because it seems like the function name is affected by how Babel does the compilation. Potentially if we find a way for Babel to compile the classes into the exact same form as before then we can leave FES as is, but I'm not sure how feasible that is.

Another option might be to make FES to look at the source file name instead of the function name to see if it originated in a file called p5.js. This will miss some valid cases, e.g.:

• if I upload a file called p5.js to the web editor, it internally renames the file when it stores it, so it would only work with external CDN links
• in an application bundled with something like Webpack, it might arbitrarily rename functions and make p5 functions not have the same name anyway

That said, it currently will also treat errors originated from _main.default as internal even if they're from other code that's compiled similarly, so the current implementation isn't perfect either, so maybe it's OK then?

[asukaminato0721]

Since that problem seems to lack hands, I decided to delay it lol. At least all other code is migrated.

[asukaminato0721]

@davepagurek I decided to postpone rewriting those Render classes, only to convert others first.

[davepagurek]

Sounds good, sorry for the delays @wuyudi. Without converting the Renderer classes the tests pass, although I think it means that our detection of internal errors for any of these converted classes will be slightly off, and it was just that the test checked that one specific class.

@limzykenneth how do you feel about merging this in so that we don't keep this PR hanging indefinitely and we make a new issue for getting FES to handle ES6 classes before we convert any more? Is it worth the slight regression of having a less clear error message for the already converted classes in the mean time?

[limzykenneth]

I think it's fine to merge for now. I don't quite remember the timeline for the next release but we probably will have some time to sort this out if possible.

[davepagurek]

Sounds good, I made an issue here #6172 to track the FES problem, and I'll merge this in now. Thanks again for your patience @wuyudi!