Closed samreid closed 4 years ago
pixelzoom
commented
4 years ago I think that a quick (15 min?), high-level discussion of this in dev meeting is fine. But there a lot of questions, and I don't think it makes sense to involve the entire development team in a deep dive. If this is a priority for PhET, then I propose that a sub-team be formed to investigate.
pixelzoom
commented
4 years ago 12/19/19 dev meeting:
Pros:
Cons:
Feasibility test 1 - what are the problems?
Feasibility test 2 - investigate production needs
Adoption:
The development team agrees that this must happen, and should be scheduled into the project management plan. The question is when.
@samreid and @jonathanolson will each put in 10 hours (sprint) on "Feasibility test 1" by mid January. Then we'll revisit and discuss with @kathy-phet if proceeding looks feasible.
samreid
commented
4 years ago I added a script that migrates repos to use import/export. It is very prototype-y, but here's how it can be used for example-sim.
npm install --save-dev webpack
npm install --save-dev webpack-cli
npm install --save-dev url-loader grunt migratenpx webpackThe result of running those steps is that it ports example-sim and all its dependencies to use import/export and builds it. It uses image loader and json loader. i18n plugin is ignored and mipmaps just use image loader at the moment. It runs in the browser for me, and works nicely. The mipmap dimensions are wrong, so the layout is incorrect. Brief investigation of highlighting/navigation/parameter popup works great for ES6 classes, not so much for places where we use inherit.
Do not run the following step without understanding that revert-migrate does a hard reset on selected repos. That being said, iteration can be done via:
revert-migrate.sh && grunt migrate && npx webpack@jonathanolson and I were surprised to see that a build (without uglify) takes only a few seconds, at least for example-sim:
real 0m2.342s user 0m3.948s sys 0m0.301s
Earlier today @pixelzoom and I wondered if using ES6 module import/export would help us with automatic imports, and it appears that it does (again, only for es6 classes, not for inherit types), and it's great that it uses aliases.
UPDATE: A reminder to myself that I had to tell Webstorm about the config file like so: https://stackoverflow.com/questions/34943631/path-aliases-for-imports-in-webstorm even though the top answer says it should be automatic.
UPDATE: I also tested by switching soundEnabled to true in a few places, and disabling assertions (due to a sizing error in the navbar), and sound is playing when I press the reset all button.
samreid
commented
4 years ago Notes from collaboration with @jonathanolson:
Compilation-free Mode proposal:
Compilation-only Mode proposal:
webpack-dev-server working, which provides live-reload. Code automatically compiles in memory (200ms) and browser automatically reloads. @jonathanolson and I agree this is promising enough that "compilation-only" mode may be sufficient.samreid
commented
4 years ago While looking into fast or incremental recompilation, we found that webpack supports a concept called hot module replacement, which allows you to edit code without reloading the entire application.
With node_modules/.bin/webpack-dev-server --hotOnly and
if ( module.hot ) {
module.hot.accept( './BarMagnetNode.js', () =>{
self.removeChild( barMagnetNode );
barMagnetNode = new BarMagnetNode( model.barMagnet, modelViewTransform );
self.addChild( barMagnetNode );
} );
}I am able to edit the code with a live, running, stateful simulation, like so:
Please note, I am editing from my IDE, not from the chrome devtools, and a rebuild is taking place before swapping the code in the browser.
Note the large logo size is because we haven't got mipmaps working yet.
pixelzoom
commented
4 years ago
- Everyone must use same local url localhost
Can you clarify what this means? Does that mean that everyone must use an identical URL for running sims out of their working copy? If so, that's going to be tricky for sharing my working copy with VMware. I currently need to use the IP address of the host machine, not localhost. So my URL looks like http://192.168.1.2/phet/, not http://localhost/phet/.
samreid
commented
4 years ago Does that mean that everyone must use an identical URL for running sims out of their working copy?
You would be able to use different hostname and port, but across developers the path would have to be the same. For instance, I serve ~/github so my path is like /example-sim but I've seen other developers serve something akin to ~/ so their path would be something like /github/example-sim. If we want "compilation-free" mode to work, it seems we would need to standardize on the Path part so that modules could be loaded in the browser without any preprocessing. However, @jonathanolson and I were stunned by the responsiveness of the incremental builds + live reload and we suspect that may be sufficiently fast for development and we may not need to pursue the "compilation-free" mode.
pixelzoom
commented
4 years ago Good to hear that incremental build + live reload is fast.
If we do need to go with "compilation-free" mode, please consider 2 things when making the decision on URL path. (1) On macOS, it's very easy (well-documented) and conventional to configure Apache to serve local websites out of ${HOME}/Sites/. (2) Some of us (me included) may have non-PhET websites that we need to serve locally. Therefore, the path to my PhET working directory on macOS is http://localhost/~cmalley/GitHub/. The path to the local copy of my company website is http://localhost/~cmalley/pixelzoom/. Etc. Trying to flatten things out to something like http://localhost/phetmarks/ would be a big pain for me, across multiple machines and VMs.
samreid
commented
4 years ago Notes from discussion with @jonathanolson.
Question: How can we support jumping around to different simulations?
a patch
samreid
commented
4 years ago Status so far:
grunt migrate which uses string replacement heuristics to convert a repo to module import/export. This script is working for the following repos:
npx webpack.perennial/bin/revert-migrate.sh to reverse the migrationWhat's working well:
Caveats for IDE support
class but it does not understand PHET_CORE/inherit or namespace.register. So code that used to read:
export default circuitConstructionKitCommon.register( 'CCKCChartNode', CCKCChartNode );would probably be changed to:
circuitConstructionKitCommon.register( 'CCKCChartNode', CCKCChartNode );
export default CCKCChartNode;Likewise, we would no longer return inherit(...), and since we benefit from ES6 classes more, we would probably convert more inherit to class where possible.
Unanswered questions
accept only supports one listener per module. I got around this by adding an Emitter with multiple listeners. But this should be reported back to webpack as a deficiency with the possibility for straightforward improvement.Next steps
grunt migrate to handle 100% repos, and refine the output to be production-ready
jonathanolson
commented
4 years ago I've added the mipmap plugin support and a moderate placeholder for the string plugin (not using other locales besides English yet). I've also transferred things over to chipper to make things non-sim-specific, and so multiple sims can be included in the watch-rebuild cycle.
This can be done with the following:
cd chipper
git checkout webpack
git pull
npm update
grunt migrate
npx webpack-dev-serverThen the following three URLs should work, and will reload on changes that affect the respective sims:
Next technical steps would be:
grunt migratejonathanolson
commented
4 years ago I've updated npm dependencies (npm update required again).
Additionally to enable split chunks and fast updates with LOTS of sims, I'm using a plugin to generate the sim dev HTML, so URLs have changed to e.g. http://localhost:9000/dist/molecule-shapes_phet.html?brand=phet&ea
jonathanolson
commented
4 years ago Investigated scalability (mixed results, may be able to improve), and got the integration with chipper proof of concept working nicely.
samreid
commented
4 years ago Notes from today's discussion:
MK: We could do compliation mode now, then save "compilation-free" for a potential later step
DB: for the precompilation step, would you have to keep track of both source PNG and output PNG.js?
JO: This would be done as part of 'grunt update', for instance.
SR: We would check in the micro-compiled steps
CM: What about clients outside of PhET? What if a client is using scenery, for instance? JO: We would still be able to build scenery and its dependencies, using webpack. Using aliases or certain plugins would "lock us in" to a certain builder, such as webpack. Compilation free "frees us up" so that we aren't locked into any particular builder. SR: Maybe ES6 modules will be easier for clients to leverage CM: We should do this at some point, but we will need to be aware that this will impact clients. AP: I'm not aware of any clients using scenery that a change like this would break.
MK: Is there something else on the horizon that we will want to move to after this?
JB: This is the module system for JS that was missing when we began. That's why we chose requirejs at the time, but this fills the hole that was missing when we began.
MK: This is built into the language, so it's not just a library that will be easily replaced.
MK: I'm excited and interested in taking further steps, thanks!
AP: How far are you? SR: 10 hours plus discussions and documentation JO: 12 hours was main development part
AP: What's next? JO: We have to decide "compilation free" or "compilation required". Updating aqua, CT, etc. Builds do work with chipper. We need string plugin. We need to make sure migrate is working nicely. Sourcemaps. Lint things, like lint rules for require statements. Be able to build scenery independently. Clean up since we have been in prototyping mode.
MK: What about maintenance releases? JO: Now maintenance releases, some sims will be in requirejs, some in ES6 modules. You will probably need to make a separate patch for each. SR: Would we rebuild and redeploy everything JO: Not necessary, this is an isolated chipper change which would be compatible with our maintenance release process, just as long as we supply appropriate patches.
MK: Is there a way to strip out assertions? JO: Uglification would still work as a postprocessing step. SR: Does the current build use webpack uglify plugin? JO: It's an independent step, but if we want sourcemaps, we would need to integrate them better.
SR: What about compilation-free vs compilation required? JO: I think it should be investigated. Maybe OK to lose aliases. We could still use webpack or webpack-dev-server to get fast loading times when iterating on a single sim. The main disadvantage to me is that we would need to 'grunt update' after changing a string/image/sound. JB: I don't feel like I have enough information to comment on this aspect. JO: I'd want to investigate compilation free mode before deciding. MK: It seems like moving toward "compilation free" makes onboarding easier, and things more idiomatic. JO: That sounds like a good next step.
AP: It seems everyone is in agreement with this general direction. "We should do this, but what's the timeline"
JB: I agree, this seems like the standard we should gravitate towards.
AP: Anyone else interested and have time to work on next steps here? JB: I have competing priorities. MK: I'm happy to help. Probably busy until January 24th SR: I'm interested in continuing development on this, though it's difficult to hit my 40/40/20 split. AP: Go ahead and JO/SR spend another 10 hours on the compilation-free part. Then maybe at the end of January, we can bring it to Kathy and have a better sense of estimate for bringing it to the whole team.
MK: I'm trying to get Gravity Force Lab into RC in the next few weeks. We should coordinate this transformation with this and other publications.
SR: Absolute vs relative?
JO: I'm not completely opposed to relative, but we would need great tool support. Absolute is slightly better for moving things.
SR: I think IDEA got relative paths wrong on a "move"
MK: I'm voting absolute
CK: I think so
SR: How will the IDE know about the root? It seems to somehow.
SR: What about sublime? DB: Chat with Jesse
samreid
commented
4 years ago I reviewed the following websites about whether we should use absolute or relative paths:
https://github.com/meteor/todos/issues/205 https://medium.com/beqode/absolute-vs-relative-import-paths-nodejs-1e4efa65a7bb https://webpack.js.org/configuration/resolve/#resolvealias
It seems conventional wisdom is that if you have a small project with low depth level or files don't move around much, then relative is best. Otherwise, absolute is best. Also reasoning that writing the script to generate absolute is easier than detecting relative paths, I decided to start with that. We can always change our minds later.
samreid
commented
4 years ago After the commit,
revert-migrate.sh && grunt migrate && grunt modulifyPrepares acid-base-solutions into compilation-free form which can load in the browser without any build step (aside from the modulify which already ran). I need help on the mipmap and the string plugins. I should also note that I created a joist branch "webpack" which is necessary to overcome some shortcomings.
UPDATE: I also learned that checkNamespaces will no longer work.
samreid
commented
4 years ago I'm struggling to get webpack to deal with absolute paths. The only thing I've had any success with is this hack which pretends absolute paths are aliases:
Index: webpack.config.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- webpack.config.js (revision 0da1030e6818ce7f0dd64f630ee9ec93273f936b)
+++ webpack.config.js (date 1578031583805)
@@ -155,7 +155,7 @@
// NOTE: Load dependencies more specifically from a sim list in the future, so we don't have such a direct dependency.
// Repos could be deleted in the future and then prior checkouts with this wouldn't work.
-const activeRepos = fs.readFileSync( path.resolve( __dirname, '../perennial/data/active-repos' ), 'utf-8' ).trim().split( /\r?\n/ ).map( s => s.trim () );
+const activeRepos = fs.readFileSync( path.resolve( __dirname, '../perennial/data/active-repos' ), 'utf-8' ).trim().split( /\r?\n/ ).map( s => s.trim() );
const reposByNamespace = {};
const aliases = {};
const entries = {};
@@ -166,7 +166,7 @@
const packageObject = JSON.parse( fs.readFileSync( packageFile, 'utf-8' ) );
if ( packageObject.phet && packageObject.phet.requirejsNamespace ) {
reposByNamespace[ packageObject.phet.requirejsNamespace ] = repo;
- aliases[ packageObject.phet.requirejsNamespace ] = path.resolve( __dirname, `../${repo}${repo === 'brand' ? '/phet' : ''}/js` );
+ aliases[ '/'+repo ] = path.resolve( __dirname, `../${repo}` );
}
}
}This seems fragile and like it may break in a future webpack. Also, there are many scattered require(SOMETHING/something) calls which were written to avoid circular loading problems, which we run into after this.
samreid
commented
4 years ago I also tried pointing entry point to "../startup.js" so it would have the correct absolute path, then importing main, but it had the same failure mode.
samreid
commented
4 years ago Maybe we should experiment using relative paths? I tested and WebStorm "Move" updates import paths correctly if and only if "search for references" is selected. That way relative paths will be easy to read, like:
// absolute
import InductorNode from '/circuit-construction-kit-common/js/view/InductorNode';
import Node from '/scenery/js/nodes/Node';
// relative
import InductorNode from './InductorNode';
import Node from '../../../scenery/js/nodes/Node';We won't need to configure any servers, including developer servers or phettest, CT, etc. Or configure our IDE to know what the "root" is. Or provide instructions to clients about how to configure their servers. I'm not too excited about ../../../scenery/ but maybe we will get used to it.
Also, now that we are using import WebStorm knows how to collapse them:
samreid
commented
4 years ago After the commits, acid-base-solutions is running compilation-free using relative paths, and building with webpack. Next steps: strings, mipmaps, entire codebase.
Some notes for when I return to strings:
// strings
import getString from '../../chipper/js/webpack/getString'; //handles locale decision and value access
import ABSStrings from '../acid-base-solutions-strings';
const acidBaseSolutionsTitleString = getString( ABSStrings, 'acid-base-solutions.title' );Or we could make the ABSStrings a rich object like:
const acidBaseSolutionsTitleString = ABSStrings.getString('acid-base-solutions.title' );Or looking up the locale
const acidBaseSolutionsTitleString = ABSStrings.values['acid-base-solutions.title']; // values is a ES5 get that uses the locale to determine the values.After the commit, English strings are loading and working nicely for acid base solutions in compilation-free mode. The iterative rebuild command is:
revert-migrate.sh && grunt migrate && grunt modulifyIf we need to prune the locales, that would be done with grunt modulify and the rest of the build would remain unchanged. Right now it just includes English.
samreid
commented
4 years ago Here's my working list of sims that aren't passing aqua in compilation-free mode:
&testSims=curve-fitting,energy-forms-and-changes,expression-exchange,graphing-lines,graphing-slope-intercept,griddle,joist,least-squares-regression,masses-and-springs,number-play,projectile-motion,rutherford-scattering,scenery-phet,sun,tambo,tappi,trig-tour,twixt,vegas,vibeCurrent progress:
grunt modulify which converts strings, images, sound, etc to JS as a preprocessing step. Mipmaps are generated for every image, and imports select between mipmap vs non-mipmap by filename. It was a great decision to investigate compilation-free mode, that makes our simulations much more flexible, makes it easy to jump between projects, sourcemaps are not needed, easier for clients to check out and run our code, etc.npx webpack or during chipper build via grunt.Next steps and questions:
grunt modulify with the desired brand before the build and leave the main code the same. (b) dynamic imports (c) consider the phet-io brand to be an add-on, so it is accomplished by building the PhET brand, then adding another build step to add in the PhET-iO parts.--lint=falserequire statement and then loading things from the namespace will no longer be necessary.grunt migrate (like the preceding bullet point) or whether it is is a good time to run grunt migrate on everything now and start committing changes to es6-modules branches so we can continue with manual steps. I'd like to discuss this point with @jonathanolson. Creating branches now will mean a lot of merging as we get closer, but would free us up for manual improvements. But maybe this should be done closer to when we want to cut over. Or maybe the es6-modules branches can be advanced enough that developers can begin working in them for sims that don't have imminent RCs.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.export default and import default but there are other features we should consider. This may be a good match for dot/Utils where we just want to require certain methods, for instance. We can also use renaming during import to distinguish between SCENERY/Line and KITE/Line sort of cases.window.require = function(){}; // TODO: https://github.com/phetsims/chipper/issues/820 this supports like require( 'DOT/Utils' ); in Vector2EDIT FROM MK:
EDIT FROM @samreid:
UPDATE FROM @jonathanolson and @samreid discussion Jan 21
UPDATES from @samreid
I'll discuss current status and next steps with @jonathanolson before we bring this back to the team.
jessegreenberg
commented
4 years ago Dev meeting 1/9/19:
To proceed with this @ariel-phet suggested we could sprint through steps to integrate this. It might happen in mid February as we could be through some publications by then and align with other timelines. Team agreed this is most efficient way to do this.
@samreid and @jonathanolson will continue with some of the above questions until then.
samreid
commented
4 years ago More notes from today's discussion:
AP: would a TypeScript investigation truly be obviated by making this switch to es6 modules? Aren’t there some other potential advantages? If not, no problem at all, I just have it on my list to potentially have someone investigate TypeScript in the future. JO: Most of the benefits of TypeScript are not given by using es6 modules SR: It’s a spectrum. RequireJS has less IDE support than ES6 modules, which again have less IDE support than typescript/flow. Typescript requires a compilation step, which ES6 modules do not require. Flow (another static type checker for JS) supports a mode where the types are indicates via comments https://flow.org/en/docs/types/comments/ and hence compilation is not necessary. After we get experience with ES6 build toolchains we may discover a way to streamline compilations across many sims, which is currently one of the hurdles to adoption for a typechecker. I think we should pursue ES6 modules and investigate both Typescript and Flow as a follow-up step.
AP: OK for SR and JO to spend more time on this, focusing on coming up with milestones and timeline for the process. We can also pick a time for “sprint”, deciding when it is the right time, and go for it. Maybe end of feb or early march? JB: A sprint for this sounds great. Feb 15 or so, when I start a new sim, it would be great to be full es6 stack when we start. AP: Are developers OK joining forces on this, or do we want something mostly working before it is hoisted on the team? Consensus: We are happy to join on this, it doesn’t need to be fully working before we cut over.
samreid
commented
4 years ago After the commits, acid-base-solutions is nearly linting.
samreid
commented
4 years ago I created a planning document here: https://docs.google.com/document/d/1_b8_DBP2Ier4fHkU_osOIjnQ6wdUtyjiifSznEzV4JM/edit
jonathanolson
commented
4 years ago For files like DOT/Utils or constants files, I think switching to also support named exports would be ideal. It's possible to have both default and named exports (since the default export literally exports something with the name default). Testing shows this works nicely:
// Utils.js
export function clamp( value, min, max ) {
// stuff
}
const Utils = {
clamp: clamp, // place it in the Utils object (extra verbose since we disallow the shortcut still)
// etc.
};
export default Utils;This allows a few different import styles, for example the default still works:
import Utils from '../dot/js/Utils.js';
Utils.clamp( ... );but also allows direct usage:
import { clamp } from '../dot/js/Utils.js';
clamp( ... );Notably either can be renamed (import Utils as DotUtilsfrom or import { clamp as someClamp }).
Additionally, this would work well with constants files:
// ABSConstants.js
const WEAK_STRENGTH_MAX = 1E2;
export const CONCENTRATION_RANGE = new RangeWithValue( 1E-3, 1, 1E-2 );
export const PH_RANGE = new Range( 0, 14 );
export const WATER_EQUILIBRIUM_CONSTANT = 1E-14;
export const WATER_CONCENTRATION = 55.6; // water concentration when it's used as a solvent, mol/L
export const WEAK_STRENGTH_RANGE = new RangeWithValue( 1E-10, WEAK_STRENGTH_MAX, 1E-7 );
export const STRONG_STRENGTH = WEAK_STRENGTH_MAX + 1; // arbitrary, but needs to be greater than weak maxand we can keep the code for compatibility with the defaults, which could be removed when we don't need the default:
const ABSConstants = {
CONCENTRATION_RANGE: CONCENTRATION_RANGE,
PH_RANGE: PH_RANGE,
WATER_EQUILIBRIUM_CONSTANT: WATER_EQUILIBRIUM_CONSTANT,
WATER_CONCENTRATION: WATER_CONCENTRATION,
WEAK_STRENGTH_RANGE: WEAK_STRENGTH_RANGE,
STRONG_STRENGTH: STRONG_STRENGTH
};
acidBaseSolutions.register( 'ABSConstants', ABSConstants );
export default ABSConstants;This allows:
import { PH_RANGE } from '../../../../acid-base-solutions/js/common/ABSConstants.js';
PH_RANGE.minAdditionally, if we prefer to NOT get rid of the ABSConstants.PH_RANGE, it is fine to import with a star:
import * as ABSConstants from '../../../../acid-base-solutions/js/common/ABSConstants.js';
ABSConstants.PH_RANGE.minSo it's my feeling that this is better long-term than our current approach. I'm unclear on whether the "getting old things to that style" is worth the effort, but I'd like to use those new features for new simulations or code.
For those cases where we need to ensure a module loads (but we don't need to assign it to a value, potentially because we use the namespace system as a workaround), we should use an empty import, e.g. in Node.js, we need to ensure Trail.js is loaded:
import '../../../scenery/js/util/Trail.js';Additionally, cyclic dependencies should work well with our system in general (at least with my testing on the cases where I've run into things), so we should be safe to do this (loading in browser and built):
// Vector2.js
import Vector3 from './Vector3.js';
// In Vector2's definition:
toVector3: function() {
return new Vector3( this.x, this.y, 0 );
},
// Vector3.js
import Vector2 from './Vector2.js';
// In Vector3's definition:
toVector2: function() {
return new Vector2( this.x, this.y );
},So empty requires should turn into empty imports, and we should have steps after to move to cyclic imports where this helps for clarity (so we can avoid using our namespace system as a workaround).
samreid
commented
4 years ago All work tracked in other issues, closing.
In https://github.com/phetsims/chipper/issues/551 we started moving simulation code to ES6, and that has been working nicely. One remaining step that we stand to benefit from is converting from requirejs to ES6 module import/export. When we began HTML5 development around 2013, ES6 was not finalized and there was no support for ES6 module import/export. Now it is the standard. It is supported on many platforms directly, and we can compile to ES5 compatibility for IE11:
When we converted from Java to HTML by way of AMD/requirejs, we lost most of our tooling support. For instance, in this code, WebStorm is unable to identify the parameters for the
Emittertype, and code navigation only takes you to line 13, not to the definition of Emitter.Similarly, in this case, WebStorm cannot find
modelViewTransform.modelToViewX, orcontainer.leftWallDoesWork, and marks them as unresolvedThis is in part because WebStorm does not know how to parse AMD modules which we are using for requirejs. ES6 module import/export has much better support--it can show parameters:
and it does a much better job highlighting and navigating:
I want to stress that this is not a minor concern--better support for highlighting, navigation, autocomplete would be a significant help in day-to-day development. I don't know what benefits might be attained in Sublime or other environments.
Furthermore, moving to the established standard pattern will be beneficial for the long term, for communicating with other developers, bringing new team members up-to-speed, interoperability with other software, future-proofing, etc.
Some questions as we proceed:
I know we are deeply entrenched in our existing pattern and got it to work for us pretty well, but my hope is that with some effort we can build a stronger foundation to help streamline future development.
Let's discuss at an upcoming developer meeting.