Skip to content
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

Review es6 template files and simLauncher.js #944

Closed
6 of 7 tasks
Denz1994 opened this issue May 7, 2020 · 3 comments
Closed
6 of 7 tasks

Review es6 template files and simLauncher.js #944

Denz1994 opened this issue May 7, 2020 · 3 comments
Assignees

Comments

@Denz1994
Copy link
Contributor

Denz1994 commented May 7, 2020

Related to #872, for reviewing sim-webpack.ejs, sim-development.html, and SimLauncher.js:

sim-webpack.js

  • Some top-level documentation indicating the file's use would be useful. I only see this file being used as a template for starting a webpack dev server (`webpackDevServer.js)

simLauncher.js

  • The dynamic imports in initializeDynamicImports assumes the path is correct. Perhaps a catch block would be useful for logging an error message for an incorrect path.
  • pendingLocks type doc is missing. {Array.<Lock>}?
  • launchBegan and launchComplete are missing type docs. {Boolean}?
  • launch() and proceedIfReady() aren't using ES6 arrow functions
  • proceedIfReady() missing jsDocs
  • createLock(object) parameter isn't described in the jsDoc. @param {Object} object?

sim-development.html
Nothing to note for this file.

Assigning to @jonathanolson to respond to the above points.

@jonathanolson
Copy link
Contributor

Looks like some other cleanup was done, but I believe I fixed up all but one thing.

The dynamic imports in initializeDynamicImports assumes the path is correct. Perhaps a catch block would be useful for logging an error message for an incorrect path.

I wasn't involved with the dynamic import setup for this. The try-catch wouldn't work with the promises, but we could use .catch() or .finally(). @samreid thoughts?

@samreid
Copy link
Member

samreid commented May 14, 2020

If an incorrect path is provided, the browser will show an error like:

simLauncher.js:21 GET http://localhost/main/brxand/phet/js/Brand.js net::ERR_ABORTED 404 (Not Found)

image

Is that sufficient?

@samreid samreid assigned Denz1994 and unassigned samreid May 14, 2020
@Denz1994
Copy link
Contributor Author

Thanks for checking @samreid. I wasn't sure how exactly this would fail (hopefully not silently) but the error message shown looks clear. I'll close this issue because the items have been addressed.

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

No branches or pull requests

3 participants