Code review for E6 module work

[jonathanolson]

(SR, CM, JB? code review, 4-8 hours) [sprintable, pre-publication] Code quality for modulify and the preprocessed modules, the revisions in the chipper build, and the webpack file: Much of the code is prototype-oriented. This is fine for tasks like grunt migrate which will ultimately be deleted after we run it once. But for grunt modulify and the JS harnesses for images, audio, text, etc should be brought to production standards.
Inline templates in modulify should be top level template files
JO: I would prefer grunt modulify would just change the repo that it is run in, and it should be included as part of grunt update

Once things are more solidified, we should review all changed chipper files and make sure they are up to our standards.

[jbphet]

@jonathanolson

Once things are more solidified, we should review all changed chipper files and make sure they are up to our standards.

This issue is assigned to me, but I'm not clear on what I need to do to address it. It almost sounds like it's not quite ready for review. @jonathanolson - can you please clarify?

[jonathanolson]

It sounds like it's not ready yet, presumably this would be done a bit more during the "sprint"? It's unclear and very broad to me.

[chrisklus]

Code to review:

in joist:

• SimLauncher.js

in chipper:

• buildRunnable.js
• buildStandalone.js
• generateDevelopmentHTML.js
• generateTestHTML.js
• getInitializationScript.js
• getStringMap.js (see Review es6 module changes to the string stack #922)
• Gruntfile.js
• modulify.js
• sortImports.js
• wepackBuild.js
• getStringModule.js (see Review es6 module changes to the string stack #922)
• load-runtime-strings.js (see Review es6 module changes to the string stack #922)
• sim-development.html
• sim-webpack.ejs

[pixelzoom]

@ariel-phet Before someone(s) are assigned to code review, we should confirm that the authors have done a review themselves using the CRC.

[jonathanolson]

reportUnusedMedia/reportUnusedStrings are commented out in buildRunnable, otherwise I believe the code I wrote is ready to review.

[pixelzoom]

3/5/20 dev meeting

• List in Code review for E6 module work #872 (comment) is probably complete.

• @samreid needs to self review modify.js first. Assigning issue to him to do this, then back to dev meeting to divvy up review.

[mattpen]

3/19 dev meeting:

@samreid still needs to self-review modulify.js

@Denz1994 will start review of buildRunnable.js, webpackBuild.js, and Gruntfile.js.

@jonathanolson said that most recent changes should be g2g, but there may be old changes that are not up to standards.

[samreid]

modulify.js is now ready for code review. Please note I added a few questions for the reviewer in comments.

[Denz1994]

I reviewed the code that was relevant to webpack in buildRunnable.js chipper/gruntfile.js and webpackBuild.js. There were only a few suggestions in the REVIEW comments added (nothing major). The grunt webpack-dev-server help doc was clear and all the options documented were tested. I will reassign to @jonathanolson to address the comments.

[samreid]

Mipmaps failed for some images, via the interlacing pattern described in #831. I wasn't sure how throw interplayed with async so I tried removing the try/catch and adding a manual test throw new Error() and it had the desired behavior. I'll commit this change.

[samreid]

Should modulifyFile (in modulify.js) be simplified with logic? This looks fine to leave as-is for me.

Thanks, I removed those comments.

[Denz1994]

Review comments for sortImports.js have been added to #930.

[Denz1994]

From Dev Meeting 04/09/2020:

I'll review generateDevelopmentHTML.js and generateTestHTML.js this week. @jonathanolson Will respond to review statements in sortImports.js.

[Denz1994]

I've started reviewing getInitializationScript.js and buildStandAlone.js. I'll add SimLauncher.js as a priority as well. These files should be reviewed by Friday.

[samreid]

SimLauncher has many changes in a branch, see phetsims/joist#621 that will be moved to master soon.

[zepumph]

From dev meeting today, @Denz1994 will wait on reviewing SimLauncher.js until @samreid merges those changes.

[samreid]

I addressed review feedback and moved SimLauncher to master.

[Denz1994]

The last file to review is modulify.js. I'll get to it this week to finish off this chip away.

[Denz1994]

All the files listed in #872 (comment) were reviewed or have their own code review issues.

[jonathanolson]

Looks like everything is in remaining other issues, closing here. Thanks!