ES6 Bootstrap

[hsab]

ES6 Bootstrap

This pull request follows the #3758 discussion; with the aim of transitioning p5 to ES6. It consists of minor changes to the build system to facilitate processing, linting and testing the library with ES6 syntax and standards, as well as major and ubiquitous syntactical modification in line with ES6 features.

It is worthy to note that these transformations are by no means complete, and do not reflect nor implement every possible feature of ES6. They are intended to facilitate a smoother transition to properly and efficiently utilize ES6 features if and when aligned with the community interests and standards. And serve to motivate contributors to gradually conform to the new style and features.

These changes are applied with the following in mind:

• Reduce syntactical ambiguity and JS idiosyncrasies.
• Increase clarity and refrain from obfuscation resulting from new features (e.g. Destructuring)
• Prioritize performance when necessary (e.g. forEach)

Moreover, there are many areas of p5 that could benefit from ES6 features beyond the scope of this PR. These features include but are not limited to:

• Generators
• Number Type Checking
• Set/Map Data-Structures
• Typed Arrays
• etc.

Unfortunately, implementing, testing and benchmarking all these features, was beyond my availability, expertise, and code familiarity.

Process

After modifying the build system to accommodate the migration, a series of transformations are applied, bootstrapped by lebab, an ES5 to ES6/ES7 transpiler. For every transformation the following process was performed:

• Apply transformation using lebab
• Ensure correct formatting using npm run lint:fix
• Manually inspect every single line that was modified and fix/adjust if needed
• Test and build using npm run grunt
• Check the example(s) of affected function(s) in the reference doc
• Commit the transformation

In the end, the library was tested against a large random selection of the examples available through p5.js-web-editor to ensure correct functionality. This was done by cloning the web editor repository and using the generated library files (p5.js and p5-min.js) instead of the CDN provided 0.8.0 version.

Issues & Concerns:

• new p5() fails to execute when using combineModules, although global mode functions as expected. The error thrown is p5 is not a constructor
• Older browsers have not been tested given that the library is built against browserlist last 2 versions and not dead. ( ES6 Transition: Concerns & Progress #3758 (comment) )
• Functionality has been only tested on Chrome and Firefox

Build System and General Changes

• Every instance of require has been changed to import (both node, e.g. fs, and p5 modules)
• Constants are imported such that syntax replicates old format: constants.TWO_PI
• Every module is exported in ES6 style
  Including src/core/helpers.js and src/core/constants.js
  With the exception of app.js still using module.exports = p5;
• Using @babel/register to properly perform mochaTest:test suite with babel as a compiler
• Replaced brfs with babel-plugin-static-fs
brfs is completely removed as a dependency
  static-fs allows for import { readFileSync } from 'fs';
• Fixed combineModules.js to properly handle the migration to import syntax
  Issue: p5 is not a constructor when using the custom bundle in instanced mode

Summary of Transformations

The first half of these commits, from 1375a43 to 0733e01 tackles the build system to properly process the transition.

The second half of these commits, from 7ad6a4d to 054f291 conforms the library to a limited set of ES6 features:

• Modules: Export/Import: 07f315d bc61203 7d1d26b 35c5433 0791e3b 361907e

• Modules: Default & Wildcard: d898fc5

• Constants: 7ad6a4d f57bd77

• Block-Scoped Variables: 7ad6a4d f57bd77

• Rest Parameter: c6329d7

• Spread Operator: b4e4b1f

• Method Properties: 70d7e54

• Property Shorthand: 9693c84

• Arrow Functions: Statement Bodies: 694177b

• Arrow Functions: Expression Bodies: 95274cb

• Class Definition: c97cc48

• Class Inheritance: c97cc48

• Template Literals: String Interpolation: b51a8a4

• Template Literals: Custom Interpolation: b51a8a4

• Default Parameter Values: e66ff6b

• String Searching: Includes: 49bf914

• Array Searching: Includes (ES7): 49bf914

• Iterators: For-Of: 054f291

[outofambit]

this looks great! i have a couple things i'd like to follow up on after this is merged, but they are super minor. thank you @hsab!

[stalgiag]

Hooray! Thanks for all of the hard work @hsab!

[lmccart]

A few of us have reviewed this and I'm going to go ahead and merge. Since it's such a large change, I didn't want this to get messy and confusing with different threads of questions and comments. All are welcome to leave comments on here or open new issues if you have them, and we can continue to break out issues as needed. Your help in testing the updated codebase is most appreciated.

We've already discussed the addition of a markdown file in the developer_docs folder that contains some of the content from @hsab's top post, so we have a record of the ES6 decisions made and future todos.

This is fantastic work @hsab. Thank you for leading this significant upgrade of p5.js!

[hsab]

Thank you all!
I'll probably won't be able to respond today, but will be available tomorrow and will also submit a PR for the dev docs. Hoping that this goes as smoothly as I expected it! :)

[limzykenneth]

Great work @hsab!

I have a particular question on the use of arrow functions to define prototype members. From what I can find it seems to affect the value of this in the member function (arrow function's this is defined when the function is evaluated, not when the object is created as such it doesn't point to the object instance created). For a discussion about it here and here.

I can't identify with a quick look whether it is a problem here or not but if the use of arrow function on prototype member isn't really doing anything other than saving bytes it seems a bit superflous to me.

[hsab]

@limzykenneth Thanks!

This is certainly a valid and concerning point. I will have a thorough look tomorrow.
Although I was aware of this behavior, I can't recall running into any issues with this.
Also, just did a quick look and it seems like exactly what you mention is happening in main.js. But these functions (_start, _draw, _setup) are critical (right?) and yet this seems to point to object instance during execution. This is quite a curious case. Definitely needs more investigation. If you have an idea of why, I would very much appreciate it.

[Spongman]

maybe it would be prudent to first carefully consider a list of changes that should be made, instead of just applying a bunch of automatic refactorings and seeing what sticks?

edit: too late, i guess.

[outofambit]

@Spongman if you have thoughts about the es6 refactoring, please open a new issue to discuss there. thanks!

[limzykenneth]

@hsab From my understanding, basically in the main.js case you pointed out, at the time the arrow function is evaluated, the value of this is the same as the value of this._start's this since they are defined in the same scope and so share the same value for this. However, when attaching to the prototype, the value of this should be defined at the point of object creation (when new Something() is called) but with arrow functions, the value of this in the function will be defined where the function is defined.

So in simpler terms, the issue will be centered around the usage of arrow functions on prototype members, the rest should not encounter problems and in my opinion ok to use arrow functions.

[lmccart]

@Spongman these changes were proposed by @hsab in #3758, which included a link to an in-progress branch for review. There was ample time for everyone to weigh in. @hsab also discussed the individual decisions for specific ES6 functionality to incorporate with me, and several other mentors in this project. As mentioned in the process above, this was not an automatic refactor, every single line was checked by hand with the design decisions we'd discussed in mind. Each ES6 change has been cleanly broken out into its own commit, so if there is a particular aspect we want to revert or modify, this should not be too hard. We are open to feedback, anyone is welcome to open issues for specific points they'd like to discuss further, and we can go from there. Due to the nature of this major change, and the concern about ending up with incomplete pull requests or significant merge conflicts, we felt it would be easiest to get the bulk of the work merged, and fine-tune individual aspects afterward.

More generally, this sort of passive aggressive comment is unproductive and out of line with our code of conduct and community statement. Our community includes many people that are new to coding, new to contributing to open source software, and/or people of underrepresented backgrounds that may often encounter exclusion, harassment, and other barriers to participation in OSS. We have made a commitment with p5 to maintain a community that is open and welcoming, prioritizing beginners and newbies, and supportive of learning.

I've unfortunately had a number of p5.js contributors mention to me that your posts over various threads have discouraged them and made them hesitant to participate. I have talked to you before about your comments and tone. I think it would be helpful if we could talk one on one over email to make sure we're both understanding each other, and the goals of this project. At this point, I must kindly ask that you refrain from posting on p5.js repositories until you and I have had that conversation. You can email me at laurenleemccarthy@gmail.com. Thank you.

[Spongman]

I’m sorry I don’t mean to sound aggressive. I was just concerned that some changes had been merged while there was still an open discussion about the performance impact of a couple of them. You had asked me to demonstrate those, and there was a subsequent discussion over the implications, but I had not seen a conclusion to that discussion, so I figured it was still open.

[sakshamsaxena]

Hey @hsab, for the below issue mentioned by you in description, could you maybe open a separate issue and mention me there ? I authored this wee little beast quite a long while ago, so maybe I can take a fresh look into it. Thanks!

new p5() fails to execute when using combineModules, although global mode functions as expected. The error thrown is p5 is not a constructor

[outofambit]

@sakshamsaxena it looks like you're already there, but for others: the issue for that is #3883 🙌