phetsims / tandem

Simulation-side code for PhET-iO
MIT License
0 stars 5 forks source link

stateToArgsForConstructor is poorly named. #287

Closed pixelzoom closed 1 year ago

pixelzoom commented 1 year ago

This keeps confusing me over and over. In IOType:

// For Dynamic Element Deserialization: converts the state object to arguments
// for a `create` function in PhetioGroup or other PhetioDynamicElementContainer creation function. Note that
// other non-serialized args (not dealt with here) may be supplied as closure variables. This function only needs
// to be implemented on IO Types whose core type is phetioDynamicElement: true, such as PhetioDynamicElementContainer
// elements.
// see https://github.com/phetsims/phet-io/blob/master/doc/phet-io-instrumentation-technical-guide.md#three-types-of-deserialization
stateToArgsForConstructor: supertype && supertype.stateToArgsForConstructor,

stateToArgsForConstructor is poorly named. These are not arguments to any constructor, including the core-type constructor. They are arguments to the creation function (createElement) of the PhetioDynamicElementContainer, as stated in the documentation.

There's also the inconsistency of "state" versus "state object", which is a problem throughout the API that has been previously reported.

So I recommended to renaming stateToArgsForConstructor to stateObjectToCreateElementArguments.

pixelzoom commented 1 year ago

While working on https://github.com/phetsims/beers-law-lab/issues/265, @samreid and I encountered this again. We like stateObjectToCreateElementArguments. @samreid is going to go for it.

samreid commented 1 year ago

Rename complete, and I spot tested a CCK state in studio. My precommit hooks are saying:

/Users/samreid/apache-document-root/main/chipper/node_modules/puppeteer-core/lib/cjs/puppeteer/node/ProductLauncher.js:120
                    throw new Error(`Could not find Chromium (rev. ${this.puppeteer.browserRevision}). This can occur if either\n` +
                          ^

Error: Could not find Chromium (rev. 1056772). This can occur if either
 1. you did not perform an installation before running the script (e.g. `npm install`) or
 2. your cache path is incorrectly configured (which is: /Users/samreid/.cache/puppeteer).
For (2), check out our guide on configuring puppeteer at https://pptr.dev/guides/configuration.
    at ChromeLauncher.resolveExecutablePath (/Users/samreid/apache-document-root/main/chipper/node_modules/puppeteer-core/lib/cjs/puppeteer/node/ProductLauncher.js:120:27)
    at ChromeLauncher.executablePath (/Users/samreid/apache-document-root/main/chipper/node_modules/puppeteer-core/lib/cjs/puppeteer/node/ChromeLauncher.js:166:25)
    at ChromeLauncher.launch (/Users/samreid/apache-document-root/main/chipper/node_modules/puppeteer-core/lib/cjs/puppeteer/node/ChromeLauncher.js:70:37)
    at async /Users/samreid/apache-document-root/main/chipper/js/scripts/hook-pre-commit-task.js:114:29
    at async /Users/samreid/apache-document-root/main/chipper/js/scripts/hook-pre-commit-task.js:101:21

Node.js v18.12.1

UPDATE: Thanks to https://stackoverflow.com/questions/68051648/could-not-find-expected-browser-chrome-locally, I tried ~/apache-document-root/main/chipper$ node node_modules/puppeteer/install.js and that seemed to work ok.

@pixelzoom do you want to review this change?

pixelzoom commented 1 year ago

👍🏻 closing