Investigate using TypeScript in our build tools and Node code.

phetsims / chipper

Tools for developing and building PhET interactive simulations.

http://scenerystack.org/

MIT License

12 stars 14 forks source link

Investigate using TypeScript in our build tools and Node code. #1272

Closed kathy-phet closed 7 months ago

kathy-phet commented 2 years ago

Liam also noted this other one: https://bun.sh/

samreid commented 2 years ago

From #1206

Deno https://deno.land/ describes itself as: Deno is a simple, modern and secure runtime for JavaScript and TypeScript that uses V8 and is built in Rust.

It has built-in support for TypeScript, and looks to have a superior package manager system to node. We have numerous other issues with the existing package management system, and will one day want to be able to use TypeScript in our build side. Deno looks like a good place to investigate. A few hours of investigation may help us rule this out, but if we can go forward, this would be an epic-level issue, so I'll label it as such and add it to the project board.

samreid commented 2 years ago

@liam-mulhall said:

It seems like more JS runtimes are cropping up: https://bun.sh/. Deno might not be the winner despite being worked on by Ryan Dahl. It would probably be a good idea to keep an eye on the various Node alternatives before we pick one.

samreid commented 2 years ago

I used a star history chart maker to compare node, deno and bun https://star-history.com/#denoland/deno&nodejs/node&Jarred-Sumner/bun&Date

UPDATE from MK 4/11/24:

samreid commented 2 years ago

I'm wondering if an easier intermediate step would be to use our existing transpiler interface and convert chipper/perennial code to TypeScript and just invoke it like node ../chipper/dist/ etc. We could potentially have our own alias that forwards calls, like: tsnode ./myscript.ts. But we would probably at least want to get decent sourcemaps. And changing a fundamental part of our toolchain like that will be nontrivial (whether going to node otherPath or tsnode or deno or https://typestrong.org/ts-node/). Importantly, would grunt work with these changes?

In https://github.com/phetsims/chipper/issues/990, all devs confirmed they are using node 14+, which supports import, which seems a better fit for TypeScript.

https://stackoverflow.com/questions/62419970/grunt-and-es6-modules-in-node-incompatible-without-the-use-of-mjs

UPDATE: grunt is our limiting factor here. It requires require statements from the grunt-cli. We could swap out grunt-cli for an alias-like function like so:

# https://stackoverflow.com/questions/7131670/make-a-bash-alias-that-takes-a-parameter
grunt2() {
    node ../chipper/js/grunt2/grunt2.mjs $1 $2 $3 $4
}

But that is an invasive change that requires everything to be modules.

To step in the right direction toward ES6 modules/typeScript without having to replace 100% of grunt at the outset, we can change Transpiler to transform import statements to require statements (in chipper/perennial only!), then point Gruntfile at chipper/dist.

In doing so, we get advantages:

Ability to upgrade incrementally
Grunt still works, build process still works
Opt-in to ES6 imports as we wish (ok to mix with require statements)
Opt-in to TypeScript as we wish (ok to mix with JS)

For these costs:

more complexity in transpiler
will tooling like WebStorm get confused if we mix and match? Questions:
What about code like SimVersion which we want to import in Node/grunt as well as in the sim? Would the transpiler output 2 forms? How will our import statements know which to take?

Anyways, here's a working prototype:

```diff Index: main/chipper/js/grunt/chipAway.js =================================================================== diff --git a/main/chipper/js/grunt/chipAway.js b/main/chipper/js/grunt/chipAway.js deleted file mode 100644 --- a/main/chipper/js/grunt/chipAway.js (revision 466b649e1f05d50e7280272a7b62d0dff9b00eba) +++ /dev/null (revision 466b649e1f05d50e7280272a7b62d0dff9b00eba) @@ -1,60 +0,0 @@ -// Copyright 2022, University of Colorado Boulder - -/** - * Produces an assignment list of responsible devs. Run from lint.js - * - * The Chip Away option provides a quick and easy method to assign devs to their respective repositories - * for ease in adopting and applying new typescript linting rules. - * Chip Away will return a markdown formatted checklist with the repository name, responsible dev, - * and number of errors. - * Response format: - * - [ ] {{REPO}}: @{{GITHUB_USERNAME}} {{NUMBER}} errors in {{NUMBER}} files. - * - * @author Sam Reid (PhET Interactive Simulations) - * @author Marla Schulz (PhET Interactive Simulations) - */ - -const fs = require( 'fs' ); -const _ = require( 'lodash' ); // eslint-disable-line require-statement-match -const path = require( 'path' ); - -/** - * @param {ESLint.LintResult[]} results - the results from eslint.lintFiles( patterns ) - * - { filePath: string, errorCount: number, warningCount: number } - * - see https://eslint.org/docs/latest/developer-guide/nodejs-api#-lintresult-type - * @returns {string} - a message with the chip-away checkboxes in GitHub markdown format, or a message describing why it - * - could not be accomplished - */ -module.exports = results => { - - // NOTE: This should never be run in a maintenance mode since this loads a file from phet-info which - // does not have its SHA tracked as a dependency. - let responsibleDevs = null; - try { - responsibleDevs = JSON.parse( fs.readFileSync( '../phet-info/sim-info/responsible_dev.json' ) ); - } - catch( e ) { - - // set responsibleDevs to an empty object if the file cannot be found or is not parseable. - // In this scenario, responsibleDev info would not be logged with other repo error info. - responsibleDevs = {}; - } - - const repos = results.map( result => path.relative( '../', result.filePath ).split( path.sep )[ 0 ] ); - const assignments = _.uniq( repos ).map( repo => { - - const filteredResults = results.filter( result => path.relative( '../', result.filePath ).split( path.sep )[ 0 ] === repo ); - const fileCount = filteredResults.filter( result => result.errorCount + result.warningCount > 0 ).length; - const errorCount = _.sum( filteredResults.map( file => file.errorCount + file.warningCount ) ); - - if ( errorCount === 0 || repo === 'perennial-alias' ) { - return null; - } - else { - const usernames = responsibleDevs[ repo ] ? responsibleDevs[ repo ].responsibleDevs.join( ', ' ) : ''; - return ` - [ ] ${repo}: ${usernames} ${errorCount} errors in ${fileCount} files.`; - } - } ); - - return assignments.filter( assignment => assignment !== null ).join( '\n' ); -}; \ No newline at end of file Index: main/chipper/js/common/Transpiler.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/chipper/js/common/Transpiler.js b/main/chipper/js/common/Transpiler.js --- a/main/chipper/js/common/Transpiler.js (revision 466b649e1f05d50e7280272a7b62d0dff9b00eba) +++ b/main/chipper/js/common/Transpiler.js (date 1659578639105) @@ -105,7 +105,11 @@ presets: [ '../chipper/node_modules/@babel/preset-typescript', '../chipper/node_modules/@babel/preset-react' - ], sourceMaps: 'inline' + ], + sourceMaps: 'inline', + plugins: sourceFile.includes( 'chipper/' ) ? [ + '../chipper/node_modules/@babel/plugin-transform-modules-commonjs' + ] : [] } ); fs.mkdirSync( path.dirname( targetPath ), { recursive: true } ); Index: main/chipper/js/grunt/chipAway.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/chipper/js/grunt/chipAway.ts b/main/chipper/js/grunt/chipAway.ts new file mode 100644 --- /dev/null (date 1659578870969) +++ b/main/chipper/js/grunt/chipAway.ts (date 1659578870969) @@ -0,0 +1,65 @@ +// Copyright 2022, University of Colorado Boulder + +/** + * Produces an assignment list of responsible devs. Run from lint.js + * + * The Chip Away option provides a quick and easy method to assign devs to their respective repositories + * for ease in adopting and applying new typescript linting rules. + * Chip Away will return a markdown formatted checklist with the repository name, responsible dev, + * and number of errors. + * Response format: + * - [ ] {{REPO}}: @{{GITHUB_USERNAME}} {{NUMBER}} errors in {{NUMBER}} files. + * + * @author Sam Reid (PhET Interactive Simulations) + * @author Marla Schulz (PhET Interactive Simulations) + */ + +const fs = require( 'fs' ); +const _ = require( 'lodash' ); // eslint-disable-line require-statement-match +const path = require( 'path' ); + +/** + * @param {ESLint.LintResult[]} results - the results from eslint.lintFiles( patterns ) + * - { filePath: string, errorCount: number, warningCount: number } + * - see https://eslint.org/docs/latest/developer-guide/nodejs-api#-lintresult-type + * @returns {string} - a message with the chip-away checkboxes in GitHub markdown format, or a message describing why it + * - could not be accomplished + */ +export default results => { + + // // NOTE: This should never be run in a maintenance mode since this loads a file from phet-info which + // // does not have its SHA tracked as a dependency. + // let responsibleDevs = null; + // try { + // responsibleDevs = JSON.parse( fs.readFileSync( '../phet-info/sim-info/responsible_dev.json' ) ); + // } + // catch( e ) { + // + // // set responsibleDevs to an empty object if the file cannot be found or is not parseable. + // // In this scenario, responsibleDev info would not be logged with other repo error info. + // responsibleDevs = {}; + // } + // + // const repos = results.map( result => path.relative( '../', result.filePath ).split( path.sep )[ 0 ] ); + // const assignments = _.uniq( repos ).map( repo => { + // + // const filteredResults = results.filter( result => path.relative( '../', result.filePath ).split( path.sep )[ 0 ] === repo ); + // const fileCount = filteredResults.filter( result => result.errorCount + result.warningCount > 0 ).length; + // const errorCount = _.sum( filteredResults.map( file => file.errorCount + file.warningCount ) ); + // + // if ( errorCount === 0 || repo === 'perennial-alias' ) { + // return null; + // } + // else { + // const usernames = responsibleDevs[ repo ] ? responsibleDevs[ repo ].responsibleDevs.join( ', ' ) : ''; + // return ` - [ ] ${repo}: ${usernames} ${errorCount} errors in ${fileCount} files.`; + // } + // } ); + // + // return assignments.filter( assignment => assignment !== null ).join( '\n' ); + console.log( 'chip away' ); + + const m: number = 7; + console.log( m ); + +}; \ No newline at end of file Index: main/chipper/js/grunt/lint.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/chipper/js/grunt/lint.js b/main/chipper/js/grunt/lint.js --- a/main/chipper/js/grunt/lint.js (revision 466b649e1f05d50e7280272a7b62d0dff9b00eba) +++ b/main/chipper/js/grunt/lint.js (date 1659578850543) @@ -12,7 +12,7 @@ const _ = require( 'lodash' ); // eslint-disable-line require-statement-match const { ESLint } = require( 'eslint' ); // eslint-disable-line require-statement-match const fs = require( 'fs' ); -const chipAway = require( './chipAway' ); +import chipAway from './chipAway.js'; const showCommandLineProgress = require( '../common/showCommandLineProgress' ); const CacheLayer = require( '../common/CacheLayer' ); @@ -173,6 +173,8 @@ } } + chipAway(); + process.chdir( cwd ); const ok = totalWarnings + totalErrors === 0; Index: main/mean-share-and-balance/Gruntfile.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/mean-share-and-balance/Gruntfile.js b/main/mean-share-and-balance/Gruntfile.js --- a/main/mean-share-and-balance/Gruntfile.js (revision 59784f82064abbdb0e6fa2d0b89a92c13f8bd197) +++ b/main/mean-share-and-balance/Gruntfile.js (date 1659578728625) @@ -3,4 +3,4 @@ /* eslint-env node */ // use chipper's gruntfile -module.exports = require( '../chipper/js/grunt/Gruntfile.js' ); +module.exports = require( '../chipper/dist/js/chipper/js/grunt/Gruntfile.js' ); // eslint-disable-line ```

~/apache-document-root/main/mean-share-and-balance$ grunt lint
Running "lint" task
chip away
7

Done.

samreid commented 2 years ago

Here's a simpler change set that accomplishes the same thing.

```diff Index: main/chipper/js/common/Transpiler.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/chipper/js/common/Transpiler.js b/main/chipper/js/common/Transpiler.js --- a/main/chipper/js/common/Transpiler.js (revision 466b649e1f05d50e7280272a7b62d0dff9b00eba) +++ b/main/chipper/js/common/Transpiler.js (date 1659587064640) @@ -97,6 +97,7 @@ * @private */ static transpileFunction( sourceFile, targetPath, text ) { + console.log( sourceFile ); const x = core.transformSync( text, { filename: sourceFile, @@ -108,8 +109,10 @@ ], sourceMaps: 'inline' } ); + const code = sourceFile.includes( '../chipper/' ) ? x.code.split( 'export default ' ).join( 'module.exports = ' ) : x.code; + fs.mkdirSync( path.dirname( targetPath ), { recursive: true } ); - fs.writeFileSync( targetPath, x.code ); + fs.writeFileSync( targetPath, code ); } // @public Index: main/mean-share-and-balance/Gruntfile.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/mean-share-and-balance/Gruntfile.js b/main/mean-share-and-balance/Gruntfile.js --- a/main/mean-share-and-balance/Gruntfile.js (revision 59784f82064abbdb0e6fa2d0b89a92c13f8bd197) +++ b/main/mean-share-and-balance/Gruntfile.js (date 1659587196834) @@ -3,4 +3,4 @@ /* eslint-env node */ // use chipper's gruntfile -module.exports = require( '../chipper/js/grunt/Gruntfile.js' ); +module.exports = require( '../chipper/dist/js/chipper/js/grunt/Gruntfile.js' ); // eslint-disable-line ```

Do that, then rename chipper/Gruntfile.js to Gruntfile.ts and run grunt or grunt lint from mean-share-and-balance. The only thing failing for that part is WebStorm can't find a lint configuration (works fine from the command line):

samreid commented 2 years ago

From today's dev meeting:

SR: OK if we start allowing *.ts in chipper? Context is https://github.com/phetsims/chipper/issues/1272

Grunt would still work but for scripts that use .ts you would have to launch from node chipper/dist/js/chipper/myscript.js Stack traces wouldn’t line up in this version Or should we wait for sprint/quarterly goal? Transpiler would remain in 100% .js so there is no chicken-and-egg problem MK: Have you thought about ts-node? SR: It would improve consistency if chipper could use TypeScript. But there isn’t a lot of change happening in chipper right now. We could wait to do this until there is larger work to be done in chipper. SR: Are developers OK using different commands/tools to run TS tasks? That seems fine. SR: 2-4 devs could do a 2 week sprint to port as much of chipper to TS as possible. OR we get TS working in chipper and sprinkle in porting over time. SR: Deno will not work with grunt. Ts-node might not work with grunt. Migrating away from grunt is not trivial. SR: I have a prototype that has grunt working with TypeScript. But it requires an additional step to work with the transpiler. MK: I recommend pointing everything to chipper/dist, that will allow us to sprinkle in TypeScript at our discretion. SR: Using chipper/dist for now would not prevent us from using Deno or TS-Node in the future. JO: Using chipper/dist for now seems better for now so we can avoid technical debt and get into TS faster. Even though it’s somewhat clunky (chipper/dist) for now. JG: Using TypeScript in chipper sounds really nice. Not sure of the amount of work to get there. MS: chipper/dist sounds like a good plan for now. AV: Yes, I don’t have enough data for a meaningful answer based on this. JB: chipper/dist sounds fine SR: The unsolved problem is a salmon banner related to linting.
KP: OK You have green light, go for it! SR: Ok, thanks

samreid commented 2 years ago

Would it be OK for chipper typescript code to import files from phet-core? Uh-oh, that will interfere with grunt’s need to transpile imports to require().

In slack, @zepumph responded:

Yes probably. We have always had quite a separation about tooling and sim code. Maybe we could move optionize and IntentionalAny to perennial-alias?

samreid commented 2 years ago

To me, it would seem reasonable that chipper could use code from phet-core (which doesn't have any external dependencies).

Uh-oh, that will interfere with grunt’s need to transpile imports to require().

The hack above detected code in chipper and automatically converted require statements to import statements (in chipper files). If using files from other repos, this heuristic won't work anymore, and we shouldn't output 2 variants of each files (with imports vs requires).

So it seems that grunt is the main bottleneck preventing us from using es6 modules or typescript. Would we want to use typescript with require()? Maybe, but it is a half measure, and we are trying to get things more consistent across our systems. And that would make it so optionize or other exports couldn't be used by require code.

We also have the constraint that all our tooling relies on grunt-cli and grunt tooling. Checking out an old version of SHAs, you would still expect grunt --brands=phet,phet-io to work perfectly. We could swap out grunt for our own alias or executable, but that is confusing and would require each developer (and server machine) tinkering with their setup to make sure it is right. Instead, we can leave grunt as an adapter layer that invokes our own processes. I tested this with clean and it is working well, using TypeScript and import. This could also be done with deno, but I think our workload will be simplified if that is considered as a potential later step, and may not be necessary if the port to TypeScript is good enough in itself.

Prototype so far:

```diff Index: js/phet-build-script.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/phet-build-script.ts b/js/phet-build-script.ts new file mode 100644 --- /dev/null (date 1660281306129) +++ b/js/phet-build-script.ts (date 1660281306129) @@ -0,0 +1,28 @@ +// Copyright 2020-2021, University of Colorado Boulder +// Entry point for phet build commands forwarded from grunt + +import * as fs from 'fs'; // eslint-disable-line bad-sim-text + +const args: string[] = process.argv.slice( 2 ); // eslint-disable-line + +const assert = ( predicate: unknown, message: string ) => { + if ( !predicate ) { + throw new Error( message ); + } +}; + +const command = args[ 0 ]; + +// https://unix.stackexchange.com/questions/573377/do-command-line-options-take-an-equals-sign-between-option-name-and-value +const repos = args.filter( arg => arg.startsWith( '--repo=' ) ).map( arg => arg.split( '=' )[ 1 ] ); +assert && assert( repos.length === 1, 'should have 1 repo' ); +const repo = repos[ 0 ]; +if ( command === 'clean' ) { + const buildDirectory = `../${repo}/build`; + + if ( fs.existsSync( buildDirectory ) ) { + fs.rmSync( buildDirectory, { recursive: true, force: true } ); + } + + fs.mkdirSync( buildDirectory ); +} \ No newline at end of file Index: js/grunt/Gruntfile.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/grunt/Gruntfile.js b/js/grunt/Gruntfile.js --- a/js/grunt/Gruntfile.js (revision f03f428a3aa6c142d45d184a41381983cb3c2da8) +++ b/js/grunt/Gruntfile.js (date 1660281293951) @@ -93,15 +93,37 @@ 'build' ] ); + const wrapPhetBuildScript = string => { + const args = string.split( ' ' ); + + const child_process = require( 'child_process' ); + + return () => { + const done = grunt.task.current.async(); + + const p = child_process.spawn( 'node', [ '../chipper/dist/js/chipper/js/phet-build-script.js', ...args ], { + cwd: process.cwd() + } ); + + p.on( 'error', error => { + grunt.fail.fatal( `Perennial task failed: ${error}` ); + done(); + } ); + p.stderr.on( 'data', data => console.log( String( data ) ) ); + p.stdout.on( 'data', data => console.log( String( data ) ) ); + p.on( 'close', code => { + if ( code !== 0 ) { + grunt.fail.fatal( `Perennial task failed with code: ${code}` ); + } + done(); + } ); + }; + }; + grunt.registerTask( 'clean', 'Erases the build/ directory and all its contents, and recreates the build/ directory', - wrapTask( async () => { - const buildDirectory = `../${repo}/build`; - if ( grunt.file.exists( buildDirectory ) ) { - grunt.file.delete( buildDirectory ); - } - grunt.file.mkdir( buildDirectory ); - } ) ); + wrapPhetBuildScript( `clean --repo=${repo}` ) + ); grunt.registerTask( 'build-images', 'Build images only', ```

Also, this strategy lets us opt-in one task at a time. Maybe lint would be good to do next, but it has usages in perennial and the lint hooks (as well as chipper). Maybe those sites can just call the new harness so they don't need to be ported all-or-none?

samreid commented 2 years ago

@marlitas and I reviewed the proposal above, and it seems promising. We would like to test on windows before committing it to master.

samreid commented 2 years ago

We reviewed the proposed implementation with @zepumph and tested on Windows. It seemed to have reasonable operational behavior on Windows (low overhead for the spawn), but @zepumph expressed concern about maintaining the grunt layer. I don't like the grunt layer at all but argued that it may be low enough cost to maintain until we are ready to break backward compatibility in our maintenance steps.

samreid commented 2 years ago

I committed the patch above. I also realized this means the transpiler process needs to be running for clean to work, so we will need to abandon and rethink this approach or transpile chipper on startup. I realized this because of

clients that clone phet sims and want to develop them. But in the readme file it does say to start the transpiler before it says you can run grunt (though the connection isnt 100% clear)
build server does not run a transpile watch process, so it will need to transpile on startup.

Therefore, it seems reasonable to transpile chipper on Gruntfile startup. I'm concerned this could cause hiccups on a build server, so this should be tested.

Also, deno would work around that problem.

samreid commented 2 years ago

Lots of red in CT from the commits above:

acid-base-solutions : build
Build failed with status code 1:
Running "report-media" task

Running "clean" task
(node:33663) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
(Use `node --trace-warnings ...` to show where the warning was created)

/data/share/phet/continuous-testing/ct-snapshots/1661167440414/chipper/dist/js/chipper/js/phet-build-script/phet-build-script.js:3
import * as fs from 'fs'; // eslint-disable-line bad-sim-text
^^^^^^

SyntaxError: Cannot use import statement outside a module
at wrapSafe (internal/modules/cjs/loader.js:979:16)
at Module._compile (internal/modules/cjs/loader.js:1027:27)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
at Module.load (internal/modules/cjs/loader.js:928:32)
at Function.Module._load (internal/modules/cjs/loader.js:769:14)
at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)
at internal/main/run_main_module.js:17:47

Fatal error: Perennial task failed with code: 1
Snapshot from 8/22/2022, 5:24:00 AM

Bayes node version is: v14.15.0, my local version is: v16.13.1. That may explain the difference?

samreid commented 2 years ago

@marlitas and I saw that a package.json in a nested directory like chipper/js/phet-build-script did not seem to be respected. We cannot change chipper/package.json since there is a lot of require code. We could output to *.mjs but that will require changes to Transpiler.js in 2 places. First, in the place that looks up the corresponding chipper/dist path, and second in pruneStaleDistFiles. This seems easy enough and maybe we should do that. The other option would be to update everyone to Node 16.

@marlitas and I would like to try the *.mjs strategy first.

samreid commented 2 years ago

I was interested to hear that deno will support importing from the npm ecosystem within the next few months. That will make it a lot easier to move in that direction. https://deno.com/blog/changes#compatibility-with-node-and-npm

samreid commented 2 years ago

In experimenting with chipping away at this, I've considered that:

Maintaining the overhead of grunt adapters would be unfortunate. (@zepumph described this eloquently)
If we know we don't want the overhead (complexity) of grunt adapters in the long run, we may wish to save time in the long run by thinking about the best long term goal, which will involve 2 parts:

the API of how we run build lifecycle commands.

Developer experience about how build lifecycle commands are invoked. Do we want a central point that runs all things? Or should things just be scripts that can be run ad-hoc?

Some examples:

grunt --brands=phet,phet-io
node ../chipper/js/scripts/build.js --brands=phet,phet-io
node ../chipper/dist/js/chipper/scripts/build.js --brands=phet,phet-io
phetbuild --brands=phet,phet-io
deno run --allow-write --allow-read ../chipper/phetbuild.ts --brands=phet,phet-io

the implementation of the build lifecycle commands

if we want a "nickname" command like grunt or phetbuild, it could be implemented as a command line executable or an alias for something like node ../chipper/etc. It would need to work on dev machines and server machines. If we want tasks to be invokable from each other and from command line, there would be some overhead (like if node ../chipper/js/lint needed a command line API as well as an export API).

My recommendation: Wait "3 months" until deno has better npm support, then prototype with it to see if it works with our npm modules. If that seems promising, then write a long-term proposal around using deno as the entry point with the "bare minimum" of lifecycle commands centralized in separate scripts. deno ../chipper/js/phetbuild/build.ts deno ../chipper/js/phetbuild/update.ts etc. Document and formalize this API so it can be kept stable for maintenance builds. Allow arbitrary (unstable) scripts outside of the stable entry points, which do not need to be stable for maintenance builds.

The main advantages of this proposal:

Eliminate having to run npm install in every repo every time, just to get grunt adapters. See https://github.com/phetsims/chipper/issues/494
Eliminate having to install npm install grunt-cli
Eliminate ever having to run npm install again in normal development
Use ES6 modules in node https://github.com/phetsims/chipper/issues/861
Ability to use TypeScript, including importing our own patterns from phet-core, like optionize etc. And our lint rules.
Addresses a security concern

The main costs of this approach:

Time to design the desired API and implementation strategy.
The large cost of porting our build tools (chipper/js/grunt is 5000+ LOC), and difficulty testing. More in perennial.
Developers and server machines will need to install deno. 3rd party developers will need to install deno to run builds. Developers will need to learn and the new command line tools.
Perennial will need a 3rd way (chipper 3.0) for maintaining sims.

samreid commented 2 years ago

Deno 1.25 was released with initial support for importing node modules. I tested it out on lodash like so:

import _ from 'npm:lodash';
console.log( _.camelCase( 'Testing the camelCasingOfTheseWORDS' ) );

and it correctly outputted:

testingTheCamelCasingOfTheseWords

I tested it on

import { ESLint } from 'npm:eslint';

const eslint = new ESLint();
const results = await eslint.lintFiles( [ "src/**/*.js" ] );
const formatter = await eslint.loadFormatter( "stylish" );
const resultText = formatter.format( results );
console.log( resultText );

with this .eslintrc

{
  "rules": {
    "semi": ["error", "always"],
    "quotes": ["error", "double"]
  }
}

And it correctly outputted:

/Users/samreid/apache-document-root/main/tempo/src/fakefile.js 1:14 error Strings must use doublequote quotes

✖ 1 problem (1 error, 0 warnings) 1 error and 0 warnings potentially fixable with the --fix option.

I invoked deno like so:

deno --unstable run --allow-env --allow-read --allow-write src/lint.ts

Observations:

Importing two of our important npm dependencies worked well out of the box
It did not require any npm install or npm update steps.
It did require a lot of deno commands and flags.

samreid commented 2 years ago

Summary:

The goal is to support TypeScript and import statements to elevate the quality and consistency in our chipper/perennial code
grunt prevents us from being able to use import statements--it is locked to using require. This also prevents us from being able to import code like optionize, IntentionalAny, etc.
We established an adapter layer that will let us port one grunt command at a time to a new runtime. However, this is accomplished through exec and introduces nontrivial complexity/overhead/adapters.
Therefore, it would be advantageous to discuss the long-term vision for what we want here before proceeding. The comment https://github.com/phetsims/chipper/issues/1272#issuecomment-1229108859 establishes the benefits and costs of switching from node/js/grunt to deno/ts. In the preceding comment comment, I confirmed that deno 1.25 npm compatibility layer looks like it will work well for our project.
I do not feel comfortable sprinting on node -> deno conversion now, for the following reasons:
- Until complete, we will have to maintain parallel branches that will be difficult to keep in sync. So this should be done at a time where we can move swiftly on the conversion, and everyone has time to get up to speed on the new tooling once ready.
- I will not be able to complete this alone, at a minimum would require time from collaborators and reviewers.
- There are "UX" questions about how we would ideally want our build process to be invoked (mainly in the preceding comment)
- I do not know that this is a priority or that we have time for this at the moment. Even in collaboration with a subgroup, we would likely introduce delays/growing pains for other devs as we proceed. To add helpful terminology to explain the scope, we can consider this like "chipper 3.0"

Summarizing questions from the above:

Do we agree that, in the long run, the benefits of this proposal outweigh the projected costs? (pros and cons summarized in https://github.com/phetsims/chipper/issues/1272#issuecomment-1229108859 )
When is the appropriate time to work on this?
Who will collaborate on this? Who will review?
What is the desired UX for builds? (see preceding comment)

I feel the Q3 investigation quarterly goal has been accomplished, and it seems appropriate to schedule discussion around Q4 planning.

samreid commented 1 year ago

In https://github.com/phetsims/perennial/issues/284#issuecomment-1347658380, we identified a refactoring issue that has led to many issues and build errors in production builds. This type error could have been caught by a type checking lint rule we already use in sim code. For example:

    async function generateReadme(): Promise<void> {
      console.log( 'readme' );
    }

    generateReadme(); // ESLint Error: Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the `void` operator.(@typescript-eslint/no-floating-promises)

As a development team, it is important that we can perform refactoring in our build tools safely. This case is an example of why we should invest in upgrading our build/deployment tools to use TypeScript.

Let's check in at an upcoming development meeting.

marlitas commented 1 year ago

CM: How does this fit into our priorities? SR: I don't think it should take priority over sim publishing backlog, but should be scheduled for once we clear that up. May hold back some chipper/perennial work until we have Deno set up as a foundation. SR: grunt is incompatible from TS, and is why we can't make some improvements there. Before making progress on Deno we should decide how we want to deal with grunt. JO: this will involve a lot of planning on how we handle the new tooling. JB: I was reading part of this article: https://www.keithcirkel.co.uk/why-we-should-stop-using-grunt/ Should we be using just NPM scripts? It would be nice to not have to rely on grunt

This will be brought up again during Quarterly/ monthly planning as it is tracked by the epic label. Moving to "done discussing"

marlitas commented 1 year ago

Chipper/Perennial testing is also connected: https://github.com/phetsims/chipper/issues/1012

samreid commented 1 year ago

After reviewing this issue, I feel we should continue with grunt as as our primary command line interface we take the first several steps forward. Redesigning our entire command line experience is not a prerequisite for making progress here. We can proceed as follows:

Transpile chipper and its dependencies (including phet-core) using CommonJS output, to chipper/dist/commonjs
Point gruntfiles to chipper/dist/commonjs.

Most of this idea has been discussed/approved/vetted in https://github.com/phetsims/chipper/issues/1272#issuecomment-1212289150, but we turned away from that approach since we didn't have a plan to output phet-core to chipper/dist/js and chipper/dist/commonjs. But maybe that is the right direction to investigate.

Our progress in moving chipper to TypeScript would simplify a future transition to Deno, if we still want that as a future step.

Keep in mind the chipper/js/scripts/transpile should remain in *.js. Also, I'm concerned about other tooling that uses grunt output-js and grunt output-js-project and grunt output-js-all. This code may be running on CT or the build server or for maintenance tasks. If we change it up so that those script cannot run without a prior chipper/js/scripts/transpile we will have to visit each site and make sure it is good. Would be good to check with @jonathanolson on this detail.

UPDATE: There are 4 places where output-js is called:

phettest: when pulling and transpiling.
perennial: Maintenance, ReleaseBranch, and outputJSAll (which is called from phettest continuous server and phettest server).

Brainstorming one way around that problem: we could commit the dist/commonjs output to avoid the "have to transpile before running the transpiler" step. However, that would be a long-term headache that you would have to commit the source file and compiled file each time which is a bad practice.

There are also other ways to start getting into typescript without abandoning grunt, and without having to pre-transpile chipper before using it to transpile other code. One way is to have grunt tasks exec deno tasks as described in https://github.com/phetsims/chipper/issues/1272#issuecomment-1213565051.

samreid commented 1 year ago

There is another way around the problem of having tooling to make sure the dependencies are transpiled, kind of like how deno does it. We can just transpile during startup. This patch demonstrates using typescript for one file in the grunt build subsystem, and doesn't require any tooling steps, committing builds or anything like that. But it does (a) add a step during startup (b) a separate transpilation output directory and (c) and importing from the harness into the alternate transpilation directory and (d) would stack traces be incorrect without a sourcemap processor?

```diff Subject: [PATCH] Minor cleanup, see https://github.com/phetsims/ph-scale/issues/288 --- Index: js/grunt/updateCopyrightDates.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/grunt/updateCopyrightDates.js b/js/grunt/updateCopyrightDates.ts rename from js/grunt/updateCopyrightDates.js rename to js/grunt/updateCopyrightDates.ts --- a/js/grunt/updateCopyrightDates.js (revision b0d5f1deace231f4362422d07ba7c608a670a65a) +++ b/js/grunt/updateCopyrightDates.ts (date 1693779213342) @@ -7,9 +7,11 @@ * @author Sam Reid (PhET Interactive Simulations) */ +const myNumber: number = 'seven'; +console.log( myNumber ); const grunt = require( 'grunt' ); -const updateCopyrightDate = require( './updateCopyrightDate' ); +// const updateCopyrightDate = require( './updateCopyrightDate' ); /** * @public @@ -17,14 +19,16 @@ * @param {function} predicate - takes a repo-relative path {string} and returns {boolean} if the path should be updated. * @returns {Promise} */ -module.exports = async function( repo, predicate = () => true ) { - let relativeFiles = []; - grunt.file.recurse( `../${repo}`, ( abspath, rootdir, subdir, filename ) => { - relativeFiles.push( `${subdir}/${filename}` ); - } ); - relativeFiles = relativeFiles.filter( file => file.startsWith( 'js/' ) ).filter( predicate ); - - for ( const relativeFile of relativeFiles ) { - await updateCopyrightDate( repo, relativeFile ); +module.exports = + function( repo, predicate = () => true ) { + let relativeFiles = []; + // grunt.file.recurse( `../${repo}`, ( abspath, rootdir, subdir, filename ) => { + // relativeFiles.push( `${subdir}/${filename}` ); + // } ); + // relativeFiles = relativeFiles.filter( file => file.startsWith( 'js/' ) ).filter( predicate ); + // + // for ( const relativeFile of relativeFiles ) { + // await updateCopyrightDate( repo, relativeFile ); + // } + console.log( 'hello my number: ' + myNumber ); } -}; Index: js/common/Transpiler.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/Transpiler.js b/js/common/Transpiler.js --- a/js/common/Transpiler.js (revision b0d5f1deace231f4362422d07ba7c608a670a65a) +++ b/js/common/Transpiler.js (date 1693778826744) @@ -33,6 +33,8 @@ class Transpiler { + static mode = 'js'; + constructor( options ) { options = _.extend( { @@ -97,7 +99,7 @@ // Note: When we upgrade to Node 16, this may no longer be necessary, see https://github.com/phetsims/chipper/issues/1272#issuecomment-1222574593 const extension = relativePath.includes( 'phet-build-script' ) ? '.mjs' : '.js'; - return Transpiler.join( root, 'chipper', 'dist', 'js', ...relativePath.split( path.sep ) ).split( suffix ).join( extension ); + return Transpiler.join( root, 'chipper', 'dist', Transpiler.mode, ...relativePath.split( path.sep ) ).split( suffix ).join( extension ); } /** @@ -115,7 +117,8 @@ // in every sim repo. This strategy is also used in transpile.js presets: [ '../chipper/node_modules/@babel/preset-typescript', - '../chipper/node_modules/@babel/preset-react' + '../chipper/node_modules/@babel/preset-react', + ...( Transpiler.mode === 'js' ? [] : [ [ '../chipper/node_modules/@babel/preset-env', { modules: 'commonjs' } ] ] ) ], sourceMaps: 'inline', Index: js/grunt/Gruntfile.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/grunt/Gruntfile.js b/js/grunt/Gruntfile.js --- a/js/grunt/Gruntfile.js (revision b0d5f1deace231f4362422d07ba7c608a670a65a) +++ b/js/grunt/Gruntfile.js (date 1693779170244) @@ -37,8 +37,12 @@ // On the build server, or if a developer wants to run a build without running a transpile watch process, // we have to transpile any dependencies run through wrapPhetBuildScript // TODO: What if TypeScript code imports other repos? See https://github.com/phetsims/chipper/issues/1272 +const oldMode = Transpiler.mode; +Transpiler.mode = 'commonjs'; +// TODO: Cache needs to be aware of modes too transpiler.transpileRepo( 'chipper' ); transpiler.transpileRepo( 'phet-core' ); +Transpiler.mode = oldMode; module.exports = function( grunt ) { const packageObject = grunt.file.readJSON( 'package.json' ); @@ -570,9 +574,8 @@ 'update-copyright-dates', 'Update the copyright dates in JS source files based on Github dates', wrapTask( async () => { - const updateCopyrightDates = require( './updateCopyrightDates' ); - - await updateCopyrightDates( repo ); + const updateCopyrightDates = require( '../../dist/commonjs/chipper/js/grunt/updateCopyrightDates' ); + updateCopyrightDates( repo ); } ) ); ```

samreid commented 1 year ago

This patch demonstrates loading optionize from a chipper/js/grunt *.ts file:

```diff Subject: [PATCH] Rename nickelIi => nickelII and cobaltIi to cobaltII, see https://github.com/phetsims/phet-io-sim-specific/issues/37 --- Index: chipper/js/grunt/updateCopyrightDates.js =================================================================== diff --git a/chipper/js/grunt/updateCopyrightDates.js b/chipper/js/grunt/updateCopyrightDates.js deleted file mode 100644 --- a/chipper/js/grunt/updateCopyrightDates.js (revision 56219dd308f4fe33b68ccff28ccb27f64407cf22) +++ /dev/null (revision 56219dd308f4fe33b68ccff28ccb27f64407cf22) @@ -1,30 +0,0 @@ -// Copyright 2015-2021, University of Colorado Boulder - -/** - * Grunt task that determines created and last modified dates from git, and updates copyright statements accordingly, - * see #403 - * - * @author Sam Reid (PhET Interactive Simulations) - */ - - -const grunt = require( 'grunt' ); -const updateCopyrightDate = require( './updateCopyrightDate' ); - -/** - * @public - * @param {string} repo - The repository name for the files to update - * @param {function} predicate - takes a repo-relative path {string} and returns {boolean} if the path should be updated. - * @returns {Promise} - */ -module.exports = async function( repo, predicate = () => true ) { - let relativeFiles = []; - grunt.file.recurse( `../${repo}`, ( abspath, rootdir, subdir, filename ) => { - relativeFiles.push( `${subdir}/${filename}` ); - } ); - relativeFiles = relativeFiles.filter( file => file.startsWith( 'js/' ) ).filter( predicate ); - - for ( const relativeFile of relativeFiles ) { - await updateCopyrightDate( repo, relativeFile ); - } -}; Index: phet-core/js/Namespace.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-core/js/Namespace.js b/phet-core/js/Namespace.js --- a/phet-core/js/Namespace.js (revision 9954ce4c5c58efc3d7a2f5284b8091bdf92fdf77) +++ b/phet-core/js/Namespace.js (date 1695253994551) @@ -15,7 +15,7 @@ this.name = name; // @public (read-only) - if ( window.phet ) { + if ( false && window.phet ) { // We already create the chipper namespace, so we just attach to it with the register function. if ( name === 'chipper' ) { window.phet.chipper.name = 'chipper'; @@ -24,9 +24,9 @@ } else { /* TODO: Ideally we should always assert this, but in PhET-iO wrapper code, multiple built modules define the - TODO: same namespace, this should be fixed in https://github.com/phetsims/phet-io-wrappers/issues/477 */ + TODO: same namespace, this should be fixed in https://github.com/phetsims/phet-io-wrappers/issues/477 */ const ignoreAssertion = !_.hasIn( window, 'phet.chipper.brand' ); - assert && !ignoreAssertion && assert( !window.phet[ name ], `namespace ${name} already exists` ); + // assert && !ignoreAssertion && assert( !window.phet[ name ], `namespace ${name} already exists` ); window.phet[ name ] = this; } } @@ -52,12 +52,12 @@ register( key, value ) { // When using hot module replacement, a module will be loaded and initialized twice, and hence its namespace.register - // function will be called twice. This should not be an assertion error. + // // function will be called twice. This should not be an assertion error. // If the key isn't compound (doesn't contain '.'), we can just look it up on this namespace if ( key.indexOf( '.' ) < 0 ) { if ( !isHMR ) { - assert && assert( !this[ key ], `${key} is already registered for namespace ${this.name}` ); + // assert && assert( !this[ key ], `${key} is already registered for namespace ${this.name}` ); } this[ key ] = value; } @@ -70,8 +70,8 @@ for ( let i = 0; i < keys.length - 1; i++ ) { // for all but the last key if ( !isHMR ) { - assert && assert( !!parent[ keys[ i ] ], - `${[ this.name ].concat( keys.slice( 0, i + 1 ) ).join( '.' )} needs to be defined to register ${key}` ); + // // assert && assert( !!parent[ keys[ i ] ], + // `${[ this.name ].concat( keys.slice( 0, i + 1 ) ).join( '.' )} needs to be defined to register ${key}` ); } parent = parent[ keys[ i ] ]; @@ -81,7 +81,7 @@ const lastKey = keys[ keys.length - 1 ]; if ( !isHMR ) { - assert && assert( !parent[ lastKey ], `${key} is already registered for namespace ${this.name}` ); + // // assert && assert( !parent[ lastKey ], `${key} is already registered for namespace ${this.name}` ); } parent[ lastKey ] = value; Index: chipper/js/common/Transpiler.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/common/Transpiler.js b/chipper/js/common/Transpiler.js --- a/chipper/js/common/Transpiler.js (revision 56219dd308f4fe33b68ccff28ccb27f64407cf22) +++ b/chipper/js/common/Transpiler.js (date 1695299287998) @@ -40,7 +40,8 @@ verbose: false, // Add extra logging silent: false, // hide all logging but error reporting, include any specified with verbose repos: [], // {string[]} additional repos to be transpiled (beyond those listed in perennial-alias/data/active-repos) - brands: [] // {sting[]} additional brands to visit in the brand repo + brands: [], // {sting[]} additional brands to visit in the brand repo + mode: 'js' // could be 'js' or 'commonjs' }, options ); // @private @@ -48,6 +49,7 @@ this.silent = options.silent; this.repos = options.repos; this.brands = options.brands; + this.mode = options.mode; // Track the status of each repo. Key= repo, value=md5 hash of contents this.status = {}; @@ -91,13 +93,13 @@ * @returns {string} * @public */ - static getTargetPath( filename ) { + getTargetPath( filename ) { const relativePath = path.relative( root, filename ); const suffix = relativePath.substring( relativePath.lastIndexOf( '.' ) ); // Note: When we upgrade to Node 16, this may no longer be necessary, see https://github.com/phetsims/chipper/issues/1272#issuecomment-1222574593 const extension = relativePath.includes( 'phet-build-script' ) ? '.mjs' : '.js'; - return Transpiler.join( root, 'chipper', 'dist', 'js', ...relativePath.split( path.sep ) ).split( suffix ).join( extension ); + return Transpiler.join( root, 'chipper', 'dist', this.mode, ...relativePath.split( path.sep ) ).split( suffix ).join( extension ); } /** @@ -107,7 +109,7 @@ * @param {string} text - file text * @private */ - static transpileFunction( sourceFile, targetPath, text ) { + transpileFunction( sourceFile, targetPath, text ) { const x = core.transformSync( text, { filename: sourceFile, @@ -115,7 +117,8 @@ // in every sim repo. This strategy is also used in transpile.js presets: [ '../chipper/node_modules/@babel/preset-typescript', - '../chipper/node_modules/@babel/preset-react' + '../chipper/node_modules/@babel/preset-react', + ...( this.mode === 'js' ? [] : [ [ '../chipper/node_modules/@babel/preset-env', { modules: 'commonjs' } ] ] ) ], sourceMaps: 'inline', @@ -201,7 +204,7 @@ // If the file has changed, transpile and update the cache. We have to choose on the spectrum between safety // and performance. In order to maintain high performance with a low error rate, we only write the transpiled file // if (a) the cache is out of date (b) there is no target file at all or (c) if the target file has been modified. - const targetPath = Transpiler.getTargetPath( filePath ); + const targetPath = this.getTargetPath( filePath ); if ( !this.status[ filePath ] || this.status[ filePath ].sourceMD5 !== hash || !fs.existsSync( targetPath ) || this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) { @@ -210,7 +213,7 @@ if ( this.verbose ) { reason = ( !this.status[ filePath ] ) ? ' (not cached)' : ( this.status[ filePath ].sourceMD5 !== hash ) ? ' (changed)' : ( !fs.existsSync( targetPath ) ) ? ' (no target)' : ( this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) ? ' (target modified)' : '???'; } - Transpiler.transpileFunction( filePath, targetPath, text ); + this.transpileFunction( filePath, targetPath, text ); this.status[ filePath ] = { sourceMD5: hash, targetMilliseconds: Transpiler.modifiedTimeMilliseconds( targetPath ) @@ -369,7 +372,7 @@ const pathExists = fs.existsSync( filePath ); if ( !pathExists ) { - const targetPath = Transpiler.getTargetPath( filePath ); + const targetPath = this.getTargetPath( filePath ); if ( fs.existsSync( targetPath ) && fs.lstatSync( targetPath ).isFile() ) { fs.unlinkSync( targetPath ); Index: chipper/js/grunt/updateCopyrightDates.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/grunt/updateCopyrightDates.ts b/chipper/js/grunt/updateCopyrightDates.ts new file mode 100644 --- /dev/null (date 1695253994520) +++ b/chipper/js/grunt/updateCopyrightDates.ts (date 1695253994520) @@ -0,0 +1,36 @@ +// Copyright 2015-2021, University of Colorado Boulder + +/** + * Grunt task that determines created and last modified dates from git, and updates copyright statements accordingly, + * see #403 + * + * @author Sam Reid (PhET Interactive Simulations) + */ + +const myNumber: number = 'seven'; +console.log( myNumber ); + +const grunt = require( 'grunt' ); +const optionize = require( '../../../phet-core/js/optionize.js' ); +console.log( optionize ); +// const updateCopyrightDate = require( './updateCopyrightDate' ); + +/** + * @public + * @param {string} repo - The repository name for the files to update + * @param {function} predicate - takes a repo-relative path {string} and returns {boolean} if the path should be updated. + * @returns {Promise} + */ +module.exports = + function( repo, predicate = () => true ) { + let relativeFiles = []; + // grunt.file.recurse( `../${repo}`, ( abspath, rootdir, subdir, filename ) => { + // relativeFiles.push( `${subdir}/${filename}` ); + // } ); + // relativeFiles = relativeFiles.filter( file => file.startsWith( 'js/' ) ).filter( predicate ); + // + // for ( const relativeFile of relativeFiles ) { + // await updateCopyrightDate( repo, relativeFile ); + // } + console.log( 'hello my number: ' + myNumber ); + } Index: chipper/js/grunt/Gruntfile.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/grunt/Gruntfile.js b/chipper/js/grunt/Gruntfile.js --- a/chipper/js/grunt/Gruntfile.js (revision 56219dd308f4fe33b68ccff28ccb27f64407cf22) +++ b/chipper/js/grunt/Gruntfile.js (date 1695254216045) @@ -9,6 +9,18 @@ * @author Jonathan Olson */ +const Transpiler = require( '../common/Transpiler' ); +const transpiler = new Transpiler( { silent: true } ); + +const commonJSTranspiler = new Transpiler( { verbose: true, mode: 'commonjs' } ); + +// On the build server, or if a developer wants to run a build without running a transpile watch process, +// we have to transpile any dependencies run through wrapPhetBuildScript +// TODO: What if TypeScript code imports other repos? See https://github.com/phetsims/chipper/issues/1272 +// TODO: Cache needs to be aware of modes too +commonJSTranspiler.transpileRepo( 'chipper' ); +commonJSTranspiler.transpileRepo( 'phet-core' ); + /////////////////////////// // NOTE: to improve performance, the vast majority of modules are lazily imported in task registrations. Even duplicating // require statements improves the load time of this file noticeably. For details, see https://github.com/phetsims/chipper/issues/1107 @@ -31,15 +43,6 @@ } ); } -const Transpiler = require( '../common/Transpiler' ); -const transpiler = new Transpiler( { silent: true } ); - -// On the build server, or if a developer wants to run a build without running a transpile watch process, -// we have to transpile any dependencies run through wrapPhetBuildScript -// TODO: What if TypeScript code imports other repos? See https://github.com/phetsims/chipper/issues/1272 -transpiler.transpileRepo( 'chipper' ); -transpiler.transpileRepo( 'phet-core' ); - module.exports = function( grunt ) { const packageObject = grunt.file.readJSON( 'package.json' ); @@ -570,9 +573,8 @@ 'update-copyright-dates', 'Update the copyright dates in JS source files based on Github dates', wrapTask( async () => { - const updateCopyrightDates = require( './updateCopyrightDates' ); - - await updateCopyrightDates( repo ); + const updateCopyrightDates = require( '../../dist/commonjs/chipper/js/grunt/updateCopyrightDates' ); + updateCopyrightDates( repo ); } ) ); ```

samreid commented 1 year ago

Other ideas to investigate:

@babel/register auto-transpiles as it requires things, and fixes stack traces, etc. Would it be compatible with grunt?
Grunt CLI 1.3 says it supports typescript: https://gruntjs.com/blog/2018-08-15-grunt-cli-1.3.0-released

samreid commented 1 year ago

This patch has improved caching and startup time. I'm looking into how to transpile the stack traces:

```diff Subject: [PATCH] Rename nickelIi => nickelII and cobaltIi to cobaltII, see https://github.com/phetsims/phet-io-sim-specific/issues/37 --- Index: chipper/js/grunt/updateCopyrightDates.js =================================================================== diff --git a/chipper/js/grunt/updateCopyrightDates.js b/chipper/js/grunt/updateCopyrightDates.js deleted file mode 100644 --- a/chipper/js/grunt/updateCopyrightDates.js (revision 56219dd308f4fe33b68ccff28ccb27f64407cf22) +++ /dev/null (revision 56219dd308f4fe33b68ccff28ccb27f64407cf22) @@ -1,30 +0,0 @@ -// Copyright 2015-2021, University of Colorado Boulder - -/** - * Grunt task that determines created and last modified dates from git, and updates copyright statements accordingly, - * see #403 - * - * @author Sam Reid (PhET Interactive Simulations) - */ - - -const grunt = require( 'grunt' ); -const updateCopyrightDate = require( './updateCopyrightDate' ); - -/** - * @public - * @param {string} repo - The repository name for the files to update - * @param {function} predicate - takes a repo-relative path {string} and returns {boolean} if the path should be updated. - * @returns {Promise} - */ -module.exports = async function( repo, predicate = () => true ) { - let relativeFiles = []; - grunt.file.recurse( `../${repo}`, ( abspath, rootdir, subdir, filename ) => { - relativeFiles.push( `${subdir}/${filename}` ); - } ); - relativeFiles = relativeFiles.filter( file => file.startsWith( 'js/' ) ).filter( predicate ); - - for ( const relativeFile of relativeFiles ) { - await updateCopyrightDate( repo, relativeFile ); - } -}; Index: phet-core/js/Namespace.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-core/js/Namespace.js b/phet-core/js/Namespace.js --- a/phet-core/js/Namespace.js (revision 9954ce4c5c58efc3d7a2f5284b8091bdf92fdf77) +++ b/phet-core/js/Namespace.js (date 1695312893917) @@ -15,7 +15,7 @@ this.name = name; // @public (read-only) - if ( window.phet ) { + if ( false && window.phet ) { // We already create the chipper namespace, so we just attach to it with the register function. if ( name === 'chipper' ) { window.phet.chipper.name = 'chipper'; @@ -24,9 +24,9 @@ } else { /* TODO: Ideally we should always assert this, but in PhET-iO wrapper code, multiple built modules define the - TODO: same namespace, this should be fixed in https://github.com/phetsims/phet-io-wrappers/issues/477 */ + TODO: same namespace, this should be fixed in https://github.com/phetsims/phet-io-wrappers/issues/477 */ const ignoreAssertion = !_.hasIn( window, 'phet.chipper.brand' ); - assert && !ignoreAssertion && assert( !window.phet[ name ], `namespace ${name} already exists` ); + // assert && !ignoreAssertion && assert( !window.phet[ name ], `namespace ${name} already exists` ); window.phet[ name ] = this; } } @@ -52,12 +52,12 @@ register( key, value ) { // When using hot module replacement, a module will be loaded and initialized twice, and hence its namespace.register - // function will be called twice. This should not be an assertion error. + // // function will be called twice. This should not be an assertion error. // If the key isn't compound (doesn't contain '.'), we can just look it up on this namespace if ( key.indexOf( '.' ) < 0 ) { if ( !isHMR ) { - assert && assert( !this[ key ], `${key} is already registered for namespace ${this.name}` ); + // assert && assert( !this[ key ], `${key} is already registered for namespace ${this.name}` ); } this[ key ] = value; } @@ -70,8 +70,8 @@ for ( let i = 0; i < keys.length - 1; i++ ) { // for all but the last key if ( !isHMR ) { - assert && assert( !!parent[ keys[ i ] ], - `${[ this.name ].concat( keys.slice( 0, i + 1 ) ).join( '.' )} needs to be defined to register ${key}` ); + // // assert && assert( !!parent[ keys[ i ] ], + // `${[ this.name ].concat( keys.slice( 0, i + 1 ) ).join( '.' )} needs to be defined to register ${key}` ); } parent = parent[ keys[ i ] ]; @@ -81,7 +81,7 @@ const lastKey = keys[ keys.length - 1 ]; if ( !isHMR ) { - assert && assert( !parent[ lastKey ], `${key} is already registered for namespace ${this.name}` ); + // // assert && assert( !parent[ lastKey ], `${key} is already registered for namespace ${this.name}` ); } parent[ lastKey ] = value; Index: chipper/js/grunt/updateCopyrightDates.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/grunt/updateCopyrightDates.ts b/chipper/js/grunt/updateCopyrightDates.ts new file mode 100644 --- /dev/null (date 1695315401097) +++ b/chipper/js/grunt/updateCopyrightDates.ts (date 1695315401097) @@ -0,0 +1,58 @@ +// Copyright 2015-2021, University of Colorado Boulder + +/** + * Grunt task that determines created and last modified dates from git, and updates copyright statements accordingly, + * see #403 + * + * @author Sam Reid (PhET Interactive Simulations) + */ + +const myNumber: number = 'seven'; +console.log( myNumber ); + +const grunt = require( 'grunt' ); +const optionize = require( '../../../phet-core/js/optionize.js' ); +console.log( optionize ); + +require( 'source-map-support' ).install( { + hookRequire: true +} ); +// const updateCopyrightDate = require( './updateCopyrightDate' ); + +Error.prepareStackTrace = function (error, structuredStackTrace) { + // Use your `transpileStacktrace` function to modify the structuredStackTrace + const newStackTrace = 'hello world'; + + + + // Return the modified stack trace as a string + return newStackTrace; +}; + +/** + * @public + * @param {string} repo - The repository name for the files to update + * @param {function} predicate - takes a repo-relative path {string} and returns {boolean} if the path should be updated. + * @returns {Promise} + */ +module.exports = + function( repo, predicate = () => true ) { + let relativeFiles = []; + // grunt.file.recurse( `../${repo}`, ( abspath, rootdir, subdir, filename ) => { + // relativeFiles.push( `${subdir}/${filename}` ); + // } ); + // relativeFiles = relativeFiles.filter( file => file.startsWith( 'js/' ) ).filter( predicate ); + // + // for ( const relativeFile of relativeFiles ) { + // await updateCopyrightDate( repo, relativeFile ); + // } + console.log( 'hello my number: ' + myNumber ); + + require( 'source-map-support' ).install( { + hookRequire: true + } ); + + + const stack = new Error( 'test error' ).stack; + console.log( stack ); + } Index: chipper/js/common/Transpiler.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/common/Transpiler.js b/chipper/js/common/Transpiler.js --- a/chipper/js/common/Transpiler.js (revision 56219dd308f4fe33b68ccff28ccb27f64407cf22) +++ b/chipper/js/common/Transpiler.js (date 1695313962474) @@ -19,7 +19,7 @@ const _ = require( 'lodash' ); // Cache status is stored in chipper/dist so if you wipe chipper/dist you also wipe the cache -const statusPath = '../chipper/dist/js-cache-status.json'; + const root = '..' + path.sep; // Directories in a sim repo that may contain things for transpilation @@ -40,14 +40,18 @@ verbose: false, // Add extra logging silent: false, // hide all logging but error reporting, include any specified with verbose repos: [], // {string[]} additional repos to be transpiled (beyond those listed in perennial-alias/data/active-repos) - brands: [] // {sting[]} additional brands to visit in the brand repo + brands: [], // {sting[]} additional brands to visit in the brand repo + mode: 'js' // could be 'js' or 'commonjs' }, options ); // @private - this.verbose = options.verbose; + this.verbose = true; this.silent = options.silent; this.repos = options.repos; this.brands = options.brands; + this.mode = options.mode; + + this.statusPath = `../chipper/dist/${this.mode}-cache-status.json`; // Track the status of each repo. Key= repo, value=md5 hash of contents this.status = {}; @@ -63,21 +67,21 @@ } // Make sure a directory exists for the cached status file - fs.mkdirSync( path.dirname( statusPath ), { recursive: true } ); + fs.mkdirSync( path.dirname( this.statusPath ), { recursive: true } ); if ( options.clean ) { !this.silent && console.log( 'cleaning...' ); - fs.writeFileSync( statusPath, JSON.stringify( {}, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( {}, null, 2 ) ); } // Load cached status try { - this.status = JSON.parse( fs.readFileSync( statusPath, 'utf-8' ) ); + this.status = JSON.parse( fs.readFileSync( this.statusPath, 'utf-8' ) ); } catch( e ) { !this.silent && console.log( 'couldn\'t parse status cache, making a clean one' ); this.status = {}; - fs.writeFileSync( statusPath, JSON.stringify( this.status, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( this.status, null, 2 ) ); } // Use the same implementation as getRepoList, but we need to read from perennial-alias since chipper should not @@ -91,13 +95,13 @@ * @returns {string} * @public */ - static getTargetPath( filename ) { + getTargetPath( filename ) { const relativePath = path.relative( root, filename ); const suffix = relativePath.substring( relativePath.lastIndexOf( '.' ) ); // Note: When we upgrade to Node 16, this may no longer be necessary, see https://github.com/phetsims/chipper/issues/1272#issuecomment-1222574593 const extension = relativePath.includes( 'phet-build-script' ) ? '.mjs' : '.js'; - return Transpiler.join( root, 'chipper', 'dist', 'js', ...relativePath.split( path.sep ) ).split( suffix ).join( extension ); + return Transpiler.join( root, 'chipper', 'dist', this.mode, ...relativePath.split( path.sep ) ).split( suffix ).join( extension ); } /** @@ -107,7 +111,7 @@ * @param {string} text - file text * @private */ - static transpileFunction( sourceFile, targetPath, text ) { + transpileFunction( sourceFile, targetPath, text ) { const x = core.transformSync( text, { filename: sourceFile, @@ -115,7 +119,8 @@ // in every sim repo. This strategy is also used in transpile.js presets: [ '../chipper/node_modules/@babel/preset-typescript', - '../chipper/node_modules/@babel/preset-react' + '../chipper/node_modules/@babel/preset-react', + ...( this.mode === 'js' ? [] : [ [ '../chipper/node_modules/@babel/preset-env', { modules: 'commonjs' } ] ] ) ], sourceMaps: 'inline', @@ -201,21 +206,29 @@ // If the file has changed, transpile and update the cache. We have to choose on the spectrum between safety // and performance. In order to maintain high performance with a low error rate, we only write the transpiled file // if (a) the cache is out of date (b) there is no target file at all or (c) if the target file has been modified. - const targetPath = Transpiler.getTargetPath( filePath ); + const targetPath = this.getTargetPath( filePath ); if ( !this.status[ filePath ] || this.status[ filePath ].sourceMD5 !== hash || !fs.existsSync( targetPath ) || this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) { try { let reason = ''; if ( this.verbose ) { - reason = ( !this.status[ filePath ] ) ? ' (not cached)' : ( this.status[ filePath ].sourceMD5 !== hash ) ? ' (changed)' : ( !fs.existsSync( targetPath ) ) ? ' (no target)' : ( this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) ? ' (target modified)' : '???'; + reason = ( !this.status[ filePath ] ) ? ' (not cached)' : + ( this.status[ filePath ].sourceMD5 !== hash ) ? ' (changed)' : + ( !fs.existsSync( targetPath ) ) ? ' (no target)' : + ( this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) ? ' (target modified)' : + '???'; + + if ( this.status[ filePath ] ) { + console.log( filePath, this.status[ filePath ].targetMilliseconds, Transpiler.modifiedTimeMilliseconds( targetPath ) ); + } } - Transpiler.transpileFunction( filePath, targetPath, text ); + this.transpileFunction( filePath, targetPath, text ); this.status[ filePath ] = { sourceMD5: hash, targetMilliseconds: Transpiler.modifiedTimeMilliseconds( targetPath ) }; - fs.writeFileSync( statusPath, JSON.stringify( this.status, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( this.status, null, 2 ) ); const now = Date.now(); const nowTimeString = new Date( now ).toLocaleTimeString(); @@ -314,7 +327,7 @@ // @private saveCache() { - fs.writeFileSync( statusPath, JSON.stringify( this.status, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( this.status, null, 2 ) ); } // @public @@ -369,7 +382,7 @@ const pathExists = fs.existsSync( filePath ); if ( !pathExists ) { - const targetPath = Transpiler.getTargetPath( filePath ); + const targetPath = this.getTargetPath( filePath ); if ( fs.existsSync( targetPath ) && fs.lstatSync( targetPath ).isFile() ) { fs.unlinkSync( targetPath ); Index: center-and-variability/Gruntfile.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/center-and-variability/Gruntfile.js b/center-and-variability/Gruntfile.js --- a/center-and-variability/Gruntfile.js (revision 242ee109e998344573cf354115bedf82969c6000) +++ b/center-and-variability/Gruntfile.js (date 1695315191823) @@ -2,5 +2,17 @@ /* eslint-env node */ +const Transpiler = require( '../chipper/js/common/Transpiler.js' ); +const commonJSTranspiler = new Transpiler( { verbose: true, mode: 'commonjs' } ); + +// On the build server, or if a developer wants to run a build without running a transpile watch process, +// we have to transpile any dependencies run through wrapPhetBuildScript +// TODO: What if TypeScript code imports other repos? See https://github.com/phetsims/chipper/issues/1272 +// TODO: Cache needs to be aware of modes too +commonJSTranspiler.transpileRepo( 'chipper' ); +commonJSTranspiler.transpileRepo( 'phet-core' ); +commonJSTranspiler.saveCache(); + + // use chipper's gruntfile -module.exports = require( '../chipper/js/grunt/Gruntfile.js' ); +module.exports = require( '../chipper/dist/commonjs/chipper/js/grunt/Gruntfile.js' ); Index: chipper/js/grunt/Gruntfile.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/grunt/Gruntfile.js b/chipper/js/grunt/Gruntfile.ts rename from chipper/js/grunt/Gruntfile.js rename to chipper/js/grunt/Gruntfile.ts --- a/chipper/js/grunt/Gruntfile.js (revision 56219dd308f4fe33b68ccff28ccb27f64407cf22) +++ b/chipper/js/grunt/Gruntfile.ts (date 1695315211332) @@ -9,6 +9,12 @@ * @author Jonathan Olson */ +const m: number = 5; +console.log( m ); + + + + /////////////////////////// // NOTE: to improve performance, the vast majority of modules are lazily imported in task registrations. Even duplicating // require statements improves the load time of this file noticeably. For details, see https://github.com/phetsims/chipper/issues/1107 Index: chipper/package.json IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/package.json b/chipper/package.json --- a/chipper/package.json (revision 56219dd308f4fe33b68ccff28ccb27f64407cf22) +++ b/chipper/package.json (date 1695314149635) @@ -48,6 +48,7 @@ "pngjs": "~6.0.0", "puppeteer": "~19.2.2", "qunit": "~2.16.0", + "source-map-support": "^0.5.21", "taffydb": "~2.7.3", "terser": "~4.6.4", "typescript": "~5.1.3", ```

samreid commented 1 year ago

This patch is working very well:

```diff Subject: [PATCH] Rename nickelIi => nickelII and cobaltIi to cobaltII, see https://github.com/phetsims/phet-io-sim-specific/issues/37 --- Index: phet-core/js/Namespace.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-core/js/Namespace.js b/phet-core/js/Namespace.js --- a/phet-core/js/Namespace.js (revision 9954ce4c5c58efc3d7a2f5284b8091bdf92fdf77) +++ b/phet-core/js/Namespace.js (date 1695312893917) @@ -15,7 +15,7 @@ this.name = name; // @public (read-only) - if ( window.phet ) { + if ( false && window.phet ) { // We already create the chipper namespace, so we just attach to it with the register function. if ( name === 'chipper' ) { window.phet.chipper.name = 'chipper'; @@ -24,9 +24,9 @@ } else { /* TODO: Ideally we should always assert this, but in PhET-iO wrapper code, multiple built modules define the - TODO: same namespace, this should be fixed in https://github.com/phetsims/phet-io-wrappers/issues/477 */ + TODO: same namespace, this should be fixed in https://github.com/phetsims/phet-io-wrappers/issues/477 */ const ignoreAssertion = !_.hasIn( window, 'phet.chipper.brand' ); - assert && !ignoreAssertion && assert( !window.phet[ name ], `namespace ${name} already exists` ); + // assert && !ignoreAssertion && assert( !window.phet[ name ], `namespace ${name} already exists` ); window.phet[ name ] = this; } } @@ -52,12 +52,12 @@ register( key, value ) { // When using hot module replacement, a module will be loaded and initialized twice, and hence its namespace.register - // function will be called twice. This should not be an assertion error. + // // function will be called twice. This should not be an assertion error. // If the key isn't compound (doesn't contain '.'), we can just look it up on this namespace if ( key.indexOf( '.' ) < 0 ) { if ( !isHMR ) { - assert && assert( !this[ key ], `${key} is already registered for namespace ${this.name}` ); + // assert && assert( !this[ key ], `${key} is already registered for namespace ${this.name}` ); } this[ key ] = value; } @@ -70,8 +70,8 @@ for ( let i = 0; i < keys.length - 1; i++ ) { // for all but the last key if ( !isHMR ) { - assert && assert( !!parent[ keys[ i ] ], - `${[ this.name ].concat( keys.slice( 0, i + 1 ) ).join( '.' )} needs to be defined to register ${key}` ); + // // assert && assert( !!parent[ keys[ i ] ], + // `${[ this.name ].concat( keys.slice( 0, i + 1 ) ).join( '.' )} needs to be defined to register ${key}` ); } parent = parent[ keys[ i ] ]; @@ -81,7 +81,7 @@ const lastKey = keys[ keys.length - 1 ]; if ( !isHMR ) { - assert && assert( !parent[ lastKey ], `${key} is already registered for namespace ${this.name}` ); + // // assert && assert( !parent[ lastKey ], `${key} is already registered for namespace ${this.name}` ); } parent[ lastKey ] = value; Index: chipper/js/common/Transpiler.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/common/Transpiler.js b/chipper/js/common/Transpiler.js --- a/chipper/js/common/Transpiler.js (revision 56219dd308f4fe33b68ccff28ccb27f64407cf22) +++ b/chipper/js/common/Transpiler.js (date 1695321398395) @@ -19,7 +19,6 @@ const _ = require( 'lodash' ); // Cache status is stored in chipper/dist so if you wipe chipper/dist you also wipe the cache -const statusPath = '../chipper/dist/js-cache-status.json'; const root = '..' + path.sep; // Directories in a sim repo that may contain things for transpilation @@ -40,14 +39,18 @@ verbose: false, // Add extra logging silent: false, // hide all logging but error reporting, include any specified with verbose repos: [], // {string[]} additional repos to be transpiled (beyond those listed in perennial-alias/data/active-repos) - brands: [] // {sting[]} additional brands to visit in the brand repo + brands: [], // {sting[]} additional brands to visit in the brand repo + mode: 'js' // could be 'js' or 'commonjs' }, options ); // @private - this.verbose = options.verbose; + this.verbose = true; this.silent = options.silent; this.repos = options.repos; this.brands = options.brands; + this.mode = options.mode; + + this.statusPath = `../chipper/dist/${this.mode}-cache-status.json`; // Track the status of each repo. Key= repo, value=md5 hash of contents this.status = {}; @@ -63,21 +66,21 @@ } // Make sure a directory exists for the cached status file - fs.mkdirSync( path.dirname( statusPath ), { recursive: true } ); + fs.mkdirSync( path.dirname( this.statusPath ), { recursive: true } ); if ( options.clean ) { !this.silent && console.log( 'cleaning...' ); - fs.writeFileSync( statusPath, JSON.stringify( {}, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( {}, null, 2 ) ); } // Load cached status try { - this.status = JSON.parse( fs.readFileSync( statusPath, 'utf-8' ) ); + this.status = JSON.parse( fs.readFileSync( this.statusPath, 'utf-8' ) ); } catch( e ) { !this.silent && console.log( 'couldn\'t parse status cache, making a clean one' ); this.status = {}; - fs.writeFileSync( statusPath, JSON.stringify( this.status, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( this.status, null, 2 ) ); } // Use the same implementation as getRepoList, but we need to read from perennial-alias since chipper should not @@ -91,13 +94,13 @@ * @returns {string} * @public */ - static getTargetPath( filename ) { + getTargetPath( filename ) { const relativePath = path.relative( root, filename ); const suffix = relativePath.substring( relativePath.lastIndexOf( '.' ) ); // Note: When we upgrade to Node 16, this may no longer be necessary, see https://github.com/phetsims/chipper/issues/1272#issuecomment-1222574593 const extension = relativePath.includes( 'phet-build-script' ) ? '.mjs' : '.js'; - return Transpiler.join( root, 'chipper', 'dist', 'js', ...relativePath.split( path.sep ) ).split( suffix ).join( extension ); + return Transpiler.join( root, 'chipper', 'dist', this.mode, ...relativePath.split( path.sep ) ).split( suffix ).join( extension ); } /** @@ -107,7 +110,7 @@ * @param {string} text - file text * @private */ - static transpileFunction( sourceFile, targetPath, text ) { + transpileFunction( sourceFile, targetPath, text ) { const x = core.transformSync( text, { filename: sourceFile, @@ -115,7 +118,8 @@ // in every sim repo. This strategy is also used in transpile.js presets: [ '../chipper/node_modules/@babel/preset-typescript', - '../chipper/node_modules/@babel/preset-react' + '../chipper/node_modules/@babel/preset-react', + ...( this.mode === 'js' ? [] : [ [ '../chipper/node_modules/@babel/preset-env', { modules: 'commonjs' } ] ] ) ], sourceMaps: 'inline', @@ -130,7 +134,15 @@ // @public static modifiedTimeMilliseconds( file ) { - return fs.statSync( file ).mtime.getTime(); + try { + return fs.statSync( file ).mtime.getTime(); + } + catch( e ) { + + // TODO: Why is this file not found? https://github.com/phetsims/chipper/issues/1272 + console.log( 'file not found: ' + file ); + return -1; + } } // @public. Delete any files in chipper/dist/js that don't have a corresponding file in the source tree @@ -201,21 +213,29 @@ // If the file has changed, transpile and update the cache. We have to choose on the spectrum between safety // and performance. In order to maintain high performance with a low error rate, we only write the transpiled file // if (a) the cache is out of date (b) there is no target file at all or (c) if the target file has been modified. - const targetPath = Transpiler.getTargetPath( filePath ); + const targetPath = this.getTargetPath( filePath ); if ( !this.status[ filePath ] || this.status[ filePath ].sourceMD5 !== hash || !fs.existsSync( targetPath ) || this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) { try { let reason = ''; if ( this.verbose ) { - reason = ( !this.status[ filePath ] ) ? ' (not cached)' : ( this.status[ filePath ].sourceMD5 !== hash ) ? ' (changed)' : ( !fs.existsSync( targetPath ) ) ? ' (no target)' : ( this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) ? ' (target modified)' : '???'; + reason = ( !this.status[ filePath ] ) ? ' (not cached)' : + ( this.status[ filePath ].sourceMD5 !== hash ) ? ' (changed)' : + ( !fs.existsSync( targetPath ) ) ? ' (no target)' : + ( this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) ? ' (target modified)' : + '???'; + + if ( this.status[ filePath ] ) { + console.log( filePath, this.status[ filePath ].targetMilliseconds, Transpiler.modifiedTimeMilliseconds( targetPath ) ); + } } - Transpiler.transpileFunction( filePath, targetPath, text ); + this.transpileFunction( filePath, targetPath, text ); this.status[ filePath ] = { sourceMD5: hash, targetMilliseconds: Transpiler.modifiedTimeMilliseconds( targetPath ) }; - fs.writeFileSync( statusPath, JSON.stringify( this.status, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( this.status, null, 2 ) ); const now = Date.now(); const nowTimeString = new Date( now ).toLocaleTimeString(); @@ -314,7 +334,7 @@ // @private saveCache() { - fs.writeFileSync( statusPath, JSON.stringify( this.status, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( this.status, null, 2 ) ); } // @public @@ -369,7 +389,7 @@ const pathExists = fs.existsSync( filePath ); if ( !pathExists ) { - const targetPath = Transpiler.getTargetPath( filePath ); + const targetPath = this.getTargetPath( filePath ); if ( fs.existsSync( targetPath ) && fs.lstatSync( targetPath ).isFile() ) { fs.unlinkSync( targetPath ); Index: center-and-variability/Gruntfile.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/center-and-variability/Gruntfile.js b/center-and-variability/Gruntfile.js --- a/center-and-variability/Gruntfile.js (revision 242ee109e998344573cf354115bedf82969c6000) +++ b/center-and-variability/Gruntfile.js (date 1695321280618) @@ -1,6 +1,15 @@ // Copyright 2022, University of Colorado Boulder /* eslint-env node */ +const Transpiler = require( '../chipper/js/common/Transpiler' ); + +const commonJSTranspiler = new Transpiler( { verbose: true, mode: 'commonjs' } ); + +// Transpile the entry points +// TODO: What if TypeScript code imports other repos? See https://github.com/phetsims/chipper/issues/1272 +commonJSTranspiler.transpileRepo( 'chipper' ); +commonJSTranspiler.transpileRepo( 'phet-core' ); +commonJSTranspiler.saveCache(); // use chipper's gruntfile -module.exports = require( '../chipper/js/grunt/Gruntfile.js' ); +module.exports = require( '../chipper/dist/commonjs/chipper/js/grunt/Gruntfile.js' ); // eslint-disable-line bad-sim-text ```

samreid commented 1 year ago

This one has a better entry point:

```diff Subject: [PATCH] Rename nickelIi => nickelII and cobaltIi to cobaltII, see https://github.com/phetsims/phet-io-sim-specific/issues/37 --- Index: chipper/js/common/Transpiler.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/common/Transpiler.js b/chipper/js/common/Transpiler.js --- a/chipper/js/common/Transpiler.js (revision 56219dd308f4fe33b68ccff28ccb27f64407cf22) +++ b/chipper/js/common/Transpiler.js (date 1695321398395) @@ -19,7 +19,6 @@ const _ = require( 'lodash' ); // Cache status is stored in chipper/dist so if you wipe chipper/dist you also wipe the cache -const statusPath = '../chipper/dist/js-cache-status.json'; const root = '..' + path.sep; // Directories in a sim repo that may contain things for transpilation @@ -40,14 +39,18 @@ verbose: false, // Add extra logging silent: false, // hide all logging but error reporting, include any specified with verbose repos: [], // {string[]} additional repos to be transpiled (beyond those listed in perennial-alias/data/active-repos) - brands: [] // {sting[]} additional brands to visit in the brand repo + brands: [], // {sting[]} additional brands to visit in the brand repo + mode: 'js' // could be 'js' or 'commonjs' }, options ); // @private - this.verbose = options.verbose; + this.verbose = true; this.silent = options.silent; this.repos = options.repos; this.brands = options.brands; + this.mode = options.mode; + + this.statusPath = `../chipper/dist/${this.mode}-cache-status.json`; // Track the status of each repo. Key= repo, value=md5 hash of contents this.status = {}; @@ -63,21 +66,21 @@ } // Make sure a directory exists for the cached status file - fs.mkdirSync( path.dirname( statusPath ), { recursive: true } ); + fs.mkdirSync( path.dirname( this.statusPath ), { recursive: true } ); if ( options.clean ) { !this.silent && console.log( 'cleaning...' ); - fs.writeFileSync( statusPath, JSON.stringify( {}, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( {}, null, 2 ) ); } // Load cached status try { - this.status = JSON.parse( fs.readFileSync( statusPath, 'utf-8' ) ); + this.status = JSON.parse( fs.readFileSync( this.statusPath, 'utf-8' ) ); } catch( e ) { !this.silent && console.log( 'couldn\'t parse status cache, making a clean one' ); this.status = {}; - fs.writeFileSync( statusPath, JSON.stringify( this.status, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( this.status, null, 2 ) ); } // Use the same implementation as getRepoList, but we need to read from perennial-alias since chipper should not @@ -91,13 +94,13 @@ * @returns {string} * @public */ - static getTargetPath( filename ) { + getTargetPath( filename ) { const relativePath = path.relative( root, filename ); const suffix = relativePath.substring( relativePath.lastIndexOf( '.' ) ); // Note: When we upgrade to Node 16, this may no longer be necessary, see https://github.com/phetsims/chipper/issues/1272#issuecomment-1222574593 const extension = relativePath.includes( 'phet-build-script' ) ? '.mjs' : '.js'; - return Transpiler.join( root, 'chipper', 'dist', 'js', ...relativePath.split( path.sep ) ).split( suffix ).join( extension ); + return Transpiler.join( root, 'chipper', 'dist', this.mode, ...relativePath.split( path.sep ) ).split( suffix ).join( extension ); } /** @@ -107,7 +110,7 @@ * @param {string} text - file text * @private */ - static transpileFunction( sourceFile, targetPath, text ) { + transpileFunction( sourceFile, targetPath, text ) { const x = core.transformSync( text, { filename: sourceFile, @@ -115,7 +118,8 @@ // in every sim repo. This strategy is also used in transpile.js presets: [ '../chipper/node_modules/@babel/preset-typescript', - '../chipper/node_modules/@babel/preset-react' + '../chipper/node_modules/@babel/preset-react', + ...( this.mode === 'js' ? [] : [ [ '../chipper/node_modules/@babel/preset-env', { modules: 'commonjs' } ] ] ) ], sourceMaps: 'inline', @@ -130,7 +134,15 @@ // @public static modifiedTimeMilliseconds( file ) { - return fs.statSync( file ).mtime.getTime(); + try { + return fs.statSync( file ).mtime.getTime(); + } + catch( e ) { + + // TODO: Why is this file not found? https://github.com/phetsims/chipper/issues/1272 + console.log( 'file not found: ' + file ); + return -1; + } } // @public. Delete any files in chipper/dist/js that don't have a corresponding file in the source tree @@ -201,21 +213,29 @@ // If the file has changed, transpile and update the cache. We have to choose on the spectrum between safety // and performance. In order to maintain high performance with a low error rate, we only write the transpiled file // if (a) the cache is out of date (b) there is no target file at all or (c) if the target file has been modified. - const targetPath = Transpiler.getTargetPath( filePath ); + const targetPath = this.getTargetPath( filePath ); if ( !this.status[ filePath ] || this.status[ filePath ].sourceMD5 !== hash || !fs.existsSync( targetPath ) || this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) { try { let reason = ''; if ( this.verbose ) { - reason = ( !this.status[ filePath ] ) ? ' (not cached)' : ( this.status[ filePath ].sourceMD5 !== hash ) ? ' (changed)' : ( !fs.existsSync( targetPath ) ) ? ' (no target)' : ( this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) ? ' (target modified)' : '???'; + reason = ( !this.status[ filePath ] ) ? ' (not cached)' : + ( this.status[ filePath ].sourceMD5 !== hash ) ? ' (changed)' : + ( !fs.existsSync( targetPath ) ) ? ' (no target)' : + ( this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) ? ' (target modified)' : + '???'; + + if ( this.status[ filePath ] ) { + console.log( filePath, this.status[ filePath ].targetMilliseconds, Transpiler.modifiedTimeMilliseconds( targetPath ) ); + } } - Transpiler.transpileFunction( filePath, targetPath, text ); + this.transpileFunction( filePath, targetPath, text ); this.status[ filePath ] = { sourceMD5: hash, targetMilliseconds: Transpiler.modifiedTimeMilliseconds( targetPath ) }; - fs.writeFileSync( statusPath, JSON.stringify( this.status, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( this.status, null, 2 ) ); const now = Date.now(); const nowTimeString = new Date( now ).toLocaleTimeString(); @@ -314,7 +334,7 @@ // @private saveCache() { - fs.writeFileSync( statusPath, JSON.stringify( this.status, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( this.status, null, 2 ) ); } // @public @@ -369,7 +389,7 @@ const pathExists = fs.existsSync( filePath ); if ( !pathExists ) { - const targetPath = Transpiler.getTargetPath( filePath ); + const targetPath = this.getTargetPath( filePath ); if ( fs.existsSync( targetPath ) && fs.lstatSync( targetPath ).isFile() ) { fs.unlinkSync( targetPath ); Index: center-and-variability/Gruntfile.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/center-and-variability/Gruntfile.js b/center-and-variability/Gruntfile.js --- a/center-and-variability/Gruntfile.js (revision 242ee109e998344573cf354115bedf82969c6000) +++ b/center-and-variability/Gruntfile.js (date 1695321655887) @@ -3,4 +3,4 @@ /* eslint-env node */ // use chipper's gruntfile -module.exports = require( '../chipper/js/grunt/Gruntfile.js' ); +module.exports = require( '../chipper/js/grunt/main.js' ); Index: chipper/js/grunt/main.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/grunt/main.js b/chipper/js/grunt/main.js new file mode 100644 --- /dev/null (date 1695325201333) +++ b/chipper/js/grunt/main.js (date 1695325201333) @@ -0,0 +1,24 @@ +// Copyright 2022, University of Colorado Boulder + +/** + * In order to support development with TypeScript for the grunt tasks, transpile before starting and then point + * to the transpiled version. + * + * Since this is the entry point for TypeScript, it and its dependencies must remain in JavaScript. + * + * @author Sam Reid (PhET Interactive Simulations) + */ + +/* eslint-env node */ +const Transpiler = require( '../../js/common/Transpiler' ); + +const commonJSTranspiler = new Transpiler( { verbose: true, mode: 'commonjs' } ); + +// Transpile the entry points +// TODO: What if TypeScript code imports other repos? See https://github.com/phetsims/chipper/issues/1272 +commonJSTranspiler.transpileRepo( 'chipper' ); +commonJSTranspiler.transpileRepo( 'phet-core' ); +commonJSTranspiler.saveCache(); + +// use chipper's gruntfile +module.exports = require( '../../dist/commonjs/chipper/js/grunt/Gruntfile.js' ); ```

samreid commented 1 year ago

@jonathanolson was working on Transpiler.js for as part of WGSL for https://github.com/phetsims/chipper/commit/6830a15b93b50334255253c67f6bc37ba58c2869, so we also checked in on the patch above.

We tested the startup overhead, and saw that on my Macbook Air M1 it was typically around 50ms-80ms of additional startup overhead when no files had changed, and 300ms-500ms when one large file (like Gruntfile.js) had changed. I believe we could reduce that further if we do transpilation more eagerly in a watch process (for devs), but the build server won't be able to use that as an optimization (no watch process). We would also want to test that on Windows since we have seen it have slower process on this side. Also, if we like everything about this except for the startup time, we could explore a faster transpiler like swc.

We also discussed that the proposal in the patch, and that it would allow us to start opting in to *.ts in our build tools, would be compatible with a future transition to deno, if and when we do that. Deno uses import instead of require, but none of our work converting to typescript would be wasted.

We also discussed the main.js harness used by grunt would not work for other (non-grunt) scripts, and we would need a solution for that. But as far as I know, most of the build server/CT/developer tasks that must be run in the normal lifecycle of development are through grunt, and we would be free to use a different startup strategy for other scripts.

But keep in mind you cannot mix and match import and require, so each dependency tree would need to use one or the other but not both.

I neglected to mention that the stack traces are not transpiled--I couldn't get that working.

Generally, @jonathanolson said it seems like a promising strategy. I think it would be good to try to get to a commit point for it.

zepumph commented 1 year ago

We have a goal this iteration to do SOMETHING (anything) with this issue in the next 2 weeks. Adding assignees and noting that likely we will want to wait until after we do https://github.com/phetsims/phet-io/issues/1974 to start this.

samreid commented 8 months ago

@marlitas indicated @matthew-blackman and I would work on this in the coming iterations.

zepumph commented 8 months ago

Over in https://github.com/phetsims/chipper/issues/1430#issuecomment-2034973604, @samreid and I found that we may need to provide custom settings to repos that use both Node and browser TypeScript, since right now, with options omitted, there is an auto discovery process that allows types to mix in the same file. See https://stackoverflow.com/a/70856713/3408502

zepumph commented 7 months ago

@samreid reviewed the patch in https://github.com/phetsims/chipper/issues/1272#issuecomment-1730119939 and it looks very promising! We also discussed how this strategy (allowing baby step conversion to typescript) is likely inline with a lot of the work we would do if going "all in" on something like Deno. For example, chipper/js/grunt/fixEOL will likely look the same as a deno TS file as it would as a piecemeal/custom TS file.

samreid commented 7 months ago

I refreshed the patch above and it is working well. I also have a demo porting lint.js to lint.ts and exercising it:

```diff Subject: [PATCH] Rename nickelIi => nickelII and cobaltIi to cobaltII, see https://github.com/phetsims/phet-io-sim-specific/issues/37 --- Index: chipper/js/grunt/lint.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/grunt/lint.js b/chipper/js/grunt/lint.ts rename from chipper/js/grunt/lint.js rename to chipper/js/grunt/lint.ts --- a/chipper/js/grunt/lint.js (revision bca8ff6d98e8b9a85a75d78ab715501ef1afa7be) +++ b/chipper/js/grunt/lint.ts (date 1712854056681) @@ -29,6 +29,9 @@ // out of the repo running the command const repoToPattern = repo => `../${repo}`; +const myValue: number = 'seven'; +console.log( 'myValue: ', myValue ); + async function consoleLogResults( results ) { // No need to have the same ESLint just to format Index: chipper/js/common/Transpiler.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/common/Transpiler.js b/chipper/js/common/Transpiler.js --- a/chipper/js/common/Transpiler.js (revision bca8ff6d98e8b9a85a75d78ab715501ef1afa7be) +++ b/chipper/js/common/Transpiler.js (date 1712853851044) @@ -25,7 +25,6 @@ const _ = require( 'lodash' ); // Cache status is stored in chipper/dist so if you wipe chipper/dist you also wipe the cache -const statusPath = '../chipper/dist/js-cache-status.json'; const root = '..' + path.sep; // Directories in a sim repo that may contain things for transpilation @@ -47,15 +46,19 @@ silent: false, // hide all logging but error reporting, include any specified with verbose repos: [], // {string[]} additional repos to be transpiled (beyond those listed in perennial-alias/data/active-repos) brands: [], // {sting[]} additional brands to visit in the brand repo - minifyWGSL: false + minifyWGSL: false, + mode: 'js' // could be 'js' or 'commonjs' }, options ); // @private - this.verbose = options.verbose; + this.verbose = true; this.silent = options.silent; this.repos = options.repos; this.brands = options.brands; this.minifyWGSL = options.minifyWGSL; + this.mode = options.mode; + + this.statusPath = `../chipper/dist/${this.mode}-cache-status.json`; // Track the status of each repo. Key= repo, value=md5 hash of contents this.status = {}; @@ -71,21 +74,21 @@ } // Make sure a directory exists for the cached status file - fs.mkdirSync( path.dirname( statusPath ), { recursive: true } ); + fs.mkdirSync( path.dirname( this.statusPath ), { recursive: true } ); if ( options.clean ) { !this.silent && console.log( 'cleaning...' ); - fs.writeFileSync( statusPath, JSON.stringify( {}, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( {}, null, 2 ) ); } // Load cached status try { - this.status = JSON.parse( fs.readFileSync( statusPath, 'utf-8' ) ); + this.status = JSON.parse( fs.readFileSync( this.statusPath, 'utf-8' ) ); } catch( e ) { !this.silent && console.log( 'couldn\'t parse status cache, making a clean one' ); this.status = {}; - fs.writeFileSync( statusPath, JSON.stringify( this.status, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( this.status, null, 2 ) ); } // Use the same implementation as getRepoList, but we need to read from perennial-alias since chipper should not @@ -99,7 +102,7 @@ * @returns {string} * @public */ - static getTargetPath( filename ) { + getTargetPath( filename ) { const relativePath = path.relative( root, filename ); const suffix = relativePath.substring( relativePath.lastIndexOf( '.' ) ); @@ -108,7 +111,7 @@ relativePath.endsWith( '.mjs' ); const extension = isMJS ? '.mjs' : '.js'; - return Transpiler.join( root, 'chipper', 'dist', 'js', ...relativePath.split( path.sep ) ).split( suffix ).join( extension ); + return Transpiler.join( root, 'chipper', 'dist', this.mode, ...relativePath.split( path.sep ) ).split( suffix ).join( extension ); } /** @@ -135,7 +138,8 @@ // in every sim repo. This strategy is also used in transpile.js presets: [ '../chipper/node_modules/@babel/preset-typescript', - '../chipper/node_modules/@babel/preset-react' + '../chipper/node_modules/@babel/preset-react', + ...( this.mode === 'js' ? [] : [ [ '../chipper/node_modules/@babel/preset-env', { modules: 'commonjs' } ] ] ) ], sourceMaps: 'inline', @@ -151,7 +155,15 @@ // @public static modifiedTimeMilliseconds( file ) { - return fs.statSync( file ).mtime.getTime(); + try { + return fs.statSync( file ).mtime.getTime(); + } + catch( e ) { + + // TODO: Why is this file not found? https://github.com/phetsims/chipper/issues/1272 + console.log( 'file not found: ' + file ); + return -1; + } } // @public. Delete any files in chipper/dist/js that don't have a corresponding file in the source tree @@ -225,21 +237,29 @@ // If the file has changed, transpile and update the cache. We have to choose on the spectrum between safety // and performance. In order to maintain high performance with a low error rate, we only write the transpiled file // if (a) the cache is out of date (b) there is no target file at all or (c) if the target file has been modified. - const targetPath = Transpiler.getTargetPath( filePath ); + const targetPath = this.getTargetPath( filePath ); if ( !this.status[ filePath ] || this.status[ filePath ].sourceMD5 !== hash || !fs.existsSync( targetPath ) || this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) { try { let reason = ''; if ( this.verbose ) { - reason = ( !this.status[ filePath ] ) ? ' (not cached)' : ( this.status[ filePath ].sourceMD5 !== hash ) ? ' (changed)' : ( !fs.existsSync( targetPath ) ) ? ' (no target)' : ( this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) ? ' (target modified)' : '???'; + reason = ( !this.status[ filePath ] ) ? ' (not cached)' : + ( this.status[ filePath ].sourceMD5 !== hash ) ? ' (changed)' : + ( !fs.existsSync( targetPath ) ) ? ' (no target)' : + ( this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) ? ' (target modified)' : + '???'; + + if ( this.status[ filePath ] ) { + console.log( filePath, this.status[ filePath ].targetMilliseconds, Transpiler.modifiedTimeMilliseconds( targetPath ) ); + } } this.transpileFunction( filePath, targetPath, text ); this.status[ filePath ] = { sourceMD5: hash, targetMilliseconds: Transpiler.modifiedTimeMilliseconds( targetPath ) }; - fs.writeFileSync( statusPath, JSON.stringify( this.status, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( this.status, null, 2 ) ); const now = Date.now(); const nowTimeString = new Date( now ).toLocaleTimeString(); @@ -344,7 +364,7 @@ // @private saveCache() { - fs.writeFileSync( statusPath, JSON.stringify( this.status, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( this.status, null, 2 ) ); } // @public @@ -399,7 +419,7 @@ const pathExists = fs.existsSync( filePath ); if ( !pathExists ) { - const targetPath = Transpiler.getTargetPath( filePath ); + const targetPath = this.getTargetPath( filePath ); if ( fs.existsSync( targetPath ) && fs.lstatSync( targetPath ).isFile() ) { fs.unlinkSync( targetPath ); Index: center-and-variability/Gruntfile.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/center-and-variability/Gruntfile.js b/center-and-variability/Gruntfile.js --- a/center-and-variability/Gruntfile.js (revision cf6cb53a9df67111a2a631b6521ce9114f27fbd7) +++ b/center-and-variability/Gruntfile.js (date 1712851271073) @@ -3,4 +3,4 @@ /* eslint-env node */ // use chipper's gruntfile -module.exports = require( '../chipper/js/grunt/Gruntfile.js' ); \ No newline at end of file +module.exports = require( '../chipper/js/grunt/main.js' ); \ No newline at end of file Index: chipper/js/grunt/main.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/grunt/main.js b/chipper/js/grunt/main.js new file mode 100644 --- /dev/null (date 1712851413600) +++ b/chipper/js/grunt/main.js (date 1712851413600) @@ -0,0 +1,24 @@ +// Copyright 2022, University of Colorado Boulder + +/** + * In order to support development with TypeScript for the grunt tasks, transpile before starting and then point + * to the transpiled version. + * + * Since this is the entry point for TypeScript, it and its dependencies must remain in JavaScript. + * + * @author Sam Reid (PhET Interactive Simulations) + */ + +/* eslint-env node */ +const Transpiler = require( '../../js/common/Transpiler' ); + +const commonJSTranspiler = new Transpiler( { verbose: true, mode: 'commonjs' } ); + +// Transpile the entry points +// TODO: What if TypeScript code imports other repos? See https://github.com/phetsims/chipper/issues/1272 +commonJSTranspiler.transpileRepo( 'chipper' ); +commonJSTranspiler.transpileRepo( 'phet-core' ); +commonJSTranspiler.saveCache(); + +// use chipper's gruntfile +module.exports = require( '../../dist/commonjs/chipper/js/grunt/Gruntfile.js' );

samreid commented 7 months ago

Patch that demonstrates using optionize from chipper grunt lint:

```diff Subject: [PATCH] Rename nickelIi => nickelII and cobaltIi to cobaltII, see https://github.com/phetsims/phet-io-sim-specific/issues/37 --- Index: chipper/js/grunt/lint.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/grunt/lint.js b/chipper/js/grunt/lint.ts rename from chipper/js/grunt/lint.js rename to chipper/js/grunt/lint.ts --- a/chipper/js/grunt/lint.js (revision bca8ff6d98e8b9a85a75d78ab715501ef1afa7be) +++ b/chipper/js/grunt/lint.ts (date 1712856038187) @@ -22,6 +22,10 @@ const assert = require( 'assert' ); const { Worker } = require( 'worker_threads' ); // eslint-disable-line require-statement-match +const optionize = require( '../../../phet-core/js/optionize.js' ); + +console.log( optionize ); + // constants const EXCLUDE_REPOS = [ 'fenster', 'decaf', 'scenery-lab-demo' ]; @@ -29,6 +33,9 @@ // out of the repo running the command const repoToPattern = repo => `../${repo}`; +const myValue: number = 'seven'; +console.log( 'myValue: ', myValue ); + async function consoleLogResults( results ) { // No need to have the same ESLint just to format Index: chipper/js/common/Transpiler.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/common/Transpiler.js b/chipper/js/common/Transpiler.js --- a/chipper/js/common/Transpiler.js (revision bca8ff6d98e8b9a85a75d78ab715501ef1afa7be) +++ b/chipper/js/common/Transpiler.js (date 1712853851044) @@ -25,7 +25,6 @@ const _ = require( 'lodash' ); // Cache status is stored in chipper/dist so if you wipe chipper/dist you also wipe the cache -const statusPath = '../chipper/dist/js-cache-status.json'; const root = '..' + path.sep; // Directories in a sim repo that may contain things for transpilation @@ -47,15 +46,19 @@ silent: false, // hide all logging but error reporting, include any specified with verbose repos: [], // {string[]} additional repos to be transpiled (beyond those listed in perennial-alias/data/active-repos) brands: [], // {sting[]} additional brands to visit in the brand repo - minifyWGSL: false + minifyWGSL: false, + mode: 'js' // could be 'js' or 'commonjs' }, options ); // @private - this.verbose = options.verbose; + this.verbose = true; this.silent = options.silent; this.repos = options.repos; this.brands = options.brands; this.minifyWGSL = options.minifyWGSL; + this.mode = options.mode; + + this.statusPath = `../chipper/dist/${this.mode}-cache-status.json`; // Track the status of each repo. Key= repo, value=md5 hash of contents this.status = {}; @@ -71,21 +74,21 @@ } // Make sure a directory exists for the cached status file - fs.mkdirSync( path.dirname( statusPath ), { recursive: true } ); + fs.mkdirSync( path.dirname( this.statusPath ), { recursive: true } ); if ( options.clean ) { !this.silent && console.log( 'cleaning...' ); - fs.writeFileSync( statusPath, JSON.stringify( {}, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( {}, null, 2 ) ); } // Load cached status try { - this.status = JSON.parse( fs.readFileSync( statusPath, 'utf-8' ) ); + this.status = JSON.parse( fs.readFileSync( this.statusPath, 'utf-8' ) ); } catch( e ) { !this.silent && console.log( 'couldn\'t parse status cache, making a clean one' ); this.status = {}; - fs.writeFileSync( statusPath, JSON.stringify( this.status, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( this.status, null, 2 ) ); } // Use the same implementation as getRepoList, but we need to read from perennial-alias since chipper should not @@ -99,7 +102,7 @@ * @returns {string} * @public */ - static getTargetPath( filename ) { + getTargetPath( filename ) { const relativePath = path.relative( root, filename ); const suffix = relativePath.substring( relativePath.lastIndexOf( '.' ) ); @@ -108,7 +111,7 @@ relativePath.endsWith( '.mjs' ); const extension = isMJS ? '.mjs' : '.js'; - return Transpiler.join( root, 'chipper', 'dist', 'js', ...relativePath.split( path.sep ) ).split( suffix ).join( extension ); + return Transpiler.join( root, 'chipper', 'dist', this.mode, ...relativePath.split( path.sep ) ).split( suffix ).join( extension ); } /** @@ -135,7 +138,8 @@ // in every sim repo. This strategy is also used in transpile.js presets: [ '../chipper/node_modules/@babel/preset-typescript', - '../chipper/node_modules/@babel/preset-react' + '../chipper/node_modules/@babel/preset-react', + ...( this.mode === 'js' ? [] : [ [ '../chipper/node_modules/@babel/preset-env', { modules: 'commonjs' } ] ] ) ], sourceMaps: 'inline', @@ -151,7 +155,15 @@ // @public static modifiedTimeMilliseconds( file ) { - return fs.statSync( file ).mtime.getTime(); + try { + return fs.statSync( file ).mtime.getTime(); + } + catch( e ) { + + // TODO: Why is this file not found? https://github.com/phetsims/chipper/issues/1272 + console.log( 'file not found: ' + file ); + return -1; + } } // @public. Delete any files in chipper/dist/js that don't have a corresponding file in the source tree @@ -225,21 +237,29 @@ // If the file has changed, transpile and update the cache. We have to choose on the spectrum between safety // and performance. In order to maintain high performance with a low error rate, we only write the transpiled file // if (a) the cache is out of date (b) there is no target file at all or (c) if the target file has been modified. - const targetPath = Transpiler.getTargetPath( filePath ); + const targetPath = this.getTargetPath( filePath ); if ( !this.status[ filePath ] || this.status[ filePath ].sourceMD5 !== hash || !fs.existsSync( targetPath ) || this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) { try { let reason = ''; if ( this.verbose ) { - reason = ( !this.status[ filePath ] ) ? ' (not cached)' : ( this.status[ filePath ].sourceMD5 !== hash ) ? ' (changed)' : ( !fs.existsSync( targetPath ) ) ? ' (no target)' : ( this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) ? ' (target modified)' : '???'; + reason = ( !this.status[ filePath ] ) ? ' (not cached)' : + ( this.status[ filePath ].sourceMD5 !== hash ) ? ' (changed)' : + ( !fs.existsSync( targetPath ) ) ? ' (no target)' : + ( this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) ? ' (target modified)' : + '???'; + + if ( this.status[ filePath ] ) { + console.log( filePath, this.status[ filePath ].targetMilliseconds, Transpiler.modifiedTimeMilliseconds( targetPath ) ); + } } this.transpileFunction( filePath, targetPath, text ); this.status[ filePath ] = { sourceMD5: hash, targetMilliseconds: Transpiler.modifiedTimeMilliseconds( targetPath ) }; - fs.writeFileSync( statusPath, JSON.stringify( this.status, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( this.status, null, 2 ) ); const now = Date.now(); const nowTimeString = new Date( now ).toLocaleTimeString(); @@ -344,7 +364,7 @@ // @private saveCache() { - fs.writeFileSync( statusPath, JSON.stringify( this.status, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( this.status, null, 2 ) ); } // @public @@ -399,7 +419,7 @@ const pathExists = fs.existsSync( filePath ); if ( !pathExists ) { - const targetPath = Transpiler.getTargetPath( filePath ); + const targetPath = this.getTargetPath( filePath ); if ( fs.existsSync( targetPath ) && fs.lstatSync( targetPath ).isFile() ) { fs.unlinkSync( targetPath ); Index: center-and-variability/Gruntfile.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/center-and-variability/Gruntfile.js b/center-and-variability/Gruntfile.js --- a/center-and-variability/Gruntfile.js (revision cf6cb53a9df67111a2a631b6521ce9114f27fbd7) +++ b/center-and-variability/Gruntfile.js (date 1712851271073) @@ -3,4 +3,4 @@ /* eslint-env node */ // use chipper's gruntfile -module.exports = require( '../chipper/js/grunt/Gruntfile.js' ); \ No newline at end of file +module.exports = require( '../chipper/js/grunt/main.js' ); \ No newline at end of file Index: phet-core/js/Namespace.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-core/js/Namespace.ts b/phet-core/js/Namespace.ts --- a/phet-core/js/Namespace.ts (revision 12eaf5707ec639e8d255d40848dc1d06029d5576) +++ b/phet-core/js/Namespace.ts (date 1712856129372) @@ -14,6 +14,11 @@ this.name = name; + // When running in NodeJS, check for the existence of window + if ( typeof window === 'undefined' ) { + return; + } + if ( window.phet ) { // We already create the chipper namespace, so we just attach to it with the register function. if ( name === 'chipper' ) { @@ -45,6 +50,11 @@ */ public register( key: string, value: T ): T { + // When running in NodeJS, check for the existence of window + if ( typeof window === 'undefined' ) { + return; + } + // When using hot module replacement, a module will be loaded and initialized twice, and hence its namespace.register // function will be called twice. This should not be an assertion error. Index: chipper/js/grunt/main.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/grunt/main.js b/chipper/js/grunt/main.js new file mode 100644 --- /dev/null (date 1712851413600) +++ b/chipper/js/grunt/main.js (date 1712851413600) @@ -0,0 +1,24 @@ +// Copyright 2022, University of Colorado Boulder + +/** + * In order to support development with TypeScript for the grunt tasks, transpile before starting and then point + * to the transpiled version. + * + * Since this is the entry point for TypeScript, it and its dependencies must remain in JavaScript. + * + * @author Sam Reid (PhET Interactive Simulations) + */ + +/* eslint-env node */ +const Transpiler = require( '../../js/common/Transpiler' ); + +const commonJSTranspiler = new Transpiler( { verbose: true, mode: 'commonjs' } ); + +// Transpile the entry points +// TODO: What if TypeScript code imports other repos? See https://github.com/phetsims/chipper/issues/1272 +commonJSTranspiler.transpileRepo( 'chipper' ); +commonJSTranspiler.transpileRepo( 'phet-core' ); +commonJSTranspiler.saveCache(); + +// use chipper's gruntfile +module.exports = require( '../../dist/commonjs/chipper/js/grunt/Gruntfile.js' ); ```

jbphet commented 7 months ago

This issue was discussed in the 4/11/2024 developer meeting. Here are some notes:

@zepumph is doing regular work in perennial and chipper and feels like it would be a significant productivity improvement to have the code in TS.
@samreid feels that he has taken it about as far as he can on his own and would like help from either @jonathanolson, @pixelzoom, or @zepumph.

See the developer notes document https://docs.google.com/document/d/11Gt3Ulalj0fCD2fFeCjPT5ni_9mM2WjkGc4ysisQmo8/edit for many more notes on this discussion.

The general conclusion is that we should definitely go for an incremental process, and @samreid will review everyone's input in the developer notes doc and we will subsequently discuss the incremental plan, and @marlitas will get this item on the planning list for the next iteration.

samreid commented 7 months ago

More changes and TODOs based on collaboration with @zepumph.

Also, the revelation that we can use import statements anywhere in the chain and everything will still work because the transpiler outputs require statements that grunt can consume.

```diff Subject: [PATCH] Rename nickelIi => nickelII and cobaltIi to cobaltII, see https://github.com/phetsims/phet-io-sim-specific/issues/37 --- Index: chipper/js/grunt/updateCopyrightDates.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/grunt/updateCopyrightDates.js b/chipper/js/grunt/updateCopyrightDates.ts rename from chipper/js/grunt/updateCopyrightDates.js rename to chipper/js/grunt/updateCopyrightDates.ts --- a/chipper/js/grunt/updateCopyrightDates.js (revision bca8ff6d98e8b9a85a75d78ab715501ef1afa7be) +++ b/chipper/js/grunt/updateCopyrightDates.ts (date 1712864717061) @@ -7,9 +7,10 @@ * @author Sam Reid (PhET Interactive Simulations) */ +import updateCopyrightDate from './updateCopyrightDate.js'; +import grunt from 'grunt'; -const grunt = require( 'grunt' ); -const updateCopyrightDate = require( './updateCopyrightDate' ); +console.log(grunt); /** * @public Index: chipper/js/common/Transpiler.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/common/Transpiler.js b/chipper/js/common/Transpiler.js --- a/chipper/js/common/Transpiler.js (revision bca8ff6d98e8b9a85a75d78ab715501ef1afa7be) +++ b/chipper/js/common/Transpiler.js (date 1712863624954) @@ -11,6 +11,8 @@ * @author Sam Reid (PhET Interactive Simulations) */ +// TODO: Move this to perennial, see https://github.com/phetsims/chipper/issues/1272 + // imports const fs = require( 'fs' ); const path = require( 'path' ); @@ -25,11 +27,12 @@ const _ = require( 'lodash' ); // Cache status is stored in chipper/dist so if you wipe chipper/dist you also wipe the cache -const statusPath = '../chipper/dist/js-cache-status.json'; const root = '..' + path.sep; // Directories in a sim repo that may contain things for transpilation // This is used for a top-down search in the initial transpilation and for filtering relevant files in the watch process +// TODO: Subdirs may be different for commonjs/perennial/chipper, see https://github.com/phetsims/chipper/issues/1272 +// TODO: Add chipper/test chipper/eslint chipper/templates and perennial/test at a minimum, see https://github.com/phetsims/chipper/issues/1272 const subdirs = [ 'js', 'images', 'mipmaps', 'sounds', 'shaders', 'common', 'wgsl', // phet-io-sim-specific has nonstandard directory structure @@ -47,15 +50,19 @@ silent: false, // hide all logging but error reporting, include any specified with verbose repos: [], // {string[]} additional repos to be transpiled (beyond those listed in perennial-alias/data/active-repos) brands: [], // {sting[]} additional brands to visit in the brand repo - minifyWGSL: false + minifyWGSL: false, + mode: 'js' // could be 'js' or 'commonjs' }, options ); // @private - this.verbose = options.verbose; + this.verbose = true; this.silent = options.silent; this.repos = options.repos; this.brands = options.brands; this.minifyWGSL = options.minifyWGSL; + this.mode = options.mode; + + this.statusPath = `../chipper/dist/${this.mode}-cache-status.json`; // Track the status of each repo. Key= repo, value=md5 hash of contents this.status = {}; @@ -71,21 +78,21 @@ } // Make sure a directory exists for the cached status file - fs.mkdirSync( path.dirname( statusPath ), { recursive: true } ); + fs.mkdirSync( path.dirname( this.statusPath ), { recursive: true } ); if ( options.clean ) { !this.silent && console.log( 'cleaning...' ); - fs.writeFileSync( statusPath, JSON.stringify( {}, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( {}, null, 2 ) ); } // Load cached status try { - this.status = JSON.parse( fs.readFileSync( statusPath, 'utf-8' ) ); + this.status = JSON.parse( fs.readFileSync( this.statusPath, 'utf-8' ) ); } catch( e ) { !this.silent && console.log( 'couldn\'t parse status cache, making a clean one' ); this.status = {}; - fs.writeFileSync( statusPath, JSON.stringify( this.status, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( this.status, null, 2 ) ); } // Use the same implementation as getRepoList, but we need to read from perennial-alias since chipper should not @@ -99,7 +106,7 @@ * @returns {string} * @public */ - static getTargetPath( filename ) { + getTargetPath( filename ) { const relativePath = path.relative( root, filename ); const suffix = relativePath.substring( relativePath.lastIndexOf( '.' ) ); @@ -108,7 +115,7 @@ relativePath.endsWith( '.mjs' ); const extension = isMJS ? '.mjs' : '.js'; - return Transpiler.join( root, 'chipper', 'dist', 'js', ...relativePath.split( path.sep ) ).split( suffix ).join( extension ); + return Transpiler.join( root, 'chipper', 'dist', this.mode, ...relativePath.split( path.sep ) ).split( suffix ).join( extension ); } /** @@ -135,7 +142,8 @@ // in every sim repo. This strategy is also used in transpile.js presets: [ '../chipper/node_modules/@babel/preset-typescript', - '../chipper/node_modules/@babel/preset-react' + '../chipper/node_modules/@babel/preset-react', + ...( this.mode === 'js' ? [] : [ [ '../chipper/node_modules/@babel/preset-env', { modules: 'commonjs' } ] ] ) ], sourceMaps: 'inline', @@ -143,6 +151,10 @@ [ '../chipper/node_modules/@babel/plugin-proposal-decorators', { version: '2022-03' } ] ] } ).code; + + // TODO: Generalize this so it can look up the appropriate path for any dependency + js = js.split( `require('winston')` ).join( `require('../../../../../../perennial-alias/node_modules/winston')` ); + console.log('hello world'); } fs.mkdirSync( path.dirname( targetPath ), { recursive: true } ); @@ -151,7 +163,15 @@ // @public static modifiedTimeMilliseconds( file ) { - return fs.statSync( file ).mtime.getTime(); + try { + return fs.statSync( file ).mtime.getTime(); + } + catch( e ) { + + // TODO: Why is this file not found? https://github.com/phetsims/chipper/issues/1272 + console.log( 'file not found: ' + file ); + return -1; + } } // @public. Delete any files in chipper/dist/js that don't have a corresponding file in the source tree @@ -225,21 +245,29 @@ // If the file has changed, transpile and update the cache. We have to choose on the spectrum between safety // and performance. In order to maintain high performance with a low error rate, we only write the transpiled file // if (a) the cache is out of date (b) there is no target file at all or (c) if the target file has been modified. - const targetPath = Transpiler.getTargetPath( filePath ); + const targetPath = this.getTargetPath( filePath ); if ( !this.status[ filePath ] || this.status[ filePath ].sourceMD5 !== hash || !fs.existsSync( targetPath ) || this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) { try { let reason = ''; if ( this.verbose ) { - reason = ( !this.status[ filePath ] ) ? ' (not cached)' : ( this.status[ filePath ].sourceMD5 !== hash ) ? ' (changed)' : ( !fs.existsSync( targetPath ) ) ? ' (no target)' : ( this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) ? ' (target modified)' : '???'; + reason = ( !this.status[ filePath ] ) ? ' (not cached)' : + ( this.status[ filePath ].sourceMD5 !== hash ) ? ' (changed)' : + ( !fs.existsSync( targetPath ) ) ? ' (no target)' : + ( this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) ? ' (target modified)' : + '???'; + + if ( this.status[ filePath ] ) { + console.log( filePath, this.status[ filePath ].targetMilliseconds, Transpiler.modifiedTimeMilliseconds( targetPath ) ); + } } this.transpileFunction( filePath, targetPath, text ); this.status[ filePath ] = { sourceMD5: hash, targetMilliseconds: Transpiler.modifiedTimeMilliseconds( targetPath ) }; - fs.writeFileSync( statusPath, JSON.stringify( this.status, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( this.status, null, 2 ) ); const now = Date.now(); const nowTimeString = new Date( now ).toLocaleTimeString(); @@ -344,7 +372,7 @@ // @private saveCache() { - fs.writeFileSync( statusPath, JSON.stringify( this.status, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( this.status, null, 2 ) ); } // @public @@ -399,7 +427,7 @@ const pathExists = fs.existsSync( filePath ); if ( !pathExists ) { - const targetPath = Transpiler.getTargetPath( filePath ); + const targetPath = this.getTargetPath( filePath ); if ( fs.existsSync( targetPath ) && fs.lstatSync( targetPath ).isFile() ) { fs.unlinkSync( targetPath ); Index: center-and-variability/Gruntfile.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/center-and-variability/Gruntfile.js b/center-and-variability/Gruntfile.js --- a/center-and-variability/Gruntfile.js (revision cf6cb53a9df67111a2a631b6521ce9114f27fbd7) +++ b/center-and-variability/Gruntfile.js (date 1712851271073) @@ -3,4 +3,4 @@ /* eslint-env node */ // use chipper's gruntfile -module.exports = require( '../chipper/js/grunt/Gruntfile.js' ); \ No newline at end of file +module.exports = require( '../chipper/js/grunt/main.js' ); \ No newline at end of file Index: phet-core/js/Namespace.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-core/js/Namespace.ts b/phet-core/js/Namespace.ts --- a/phet-core/js/Namespace.ts (revision 12eaf5707ec639e8d255d40848dc1d06029d5576) +++ b/phet-core/js/Namespace.ts (date 1712856129372) @@ -14,6 +14,11 @@ this.name = name; + // When running in NodeJS, check for the existence of window + if ( typeof window === 'undefined' ) { + return; + } + if ( window.phet ) { // We already create the chipper namespace, so we just attach to it with the register function. if ( name === 'chipper' ) { @@ -45,6 +50,11 @@ */ public register( key: string, value: T ): T { + // When running in NodeJS, check for the existence of window + if ( typeof window === 'undefined' ) { + return; + } + // When using hot module replacement, a module will be loaded and initialized twice, and hence its namespace.register // function will be called twice. This should not be an assertion error. Index: chipper/js/grunt/main.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/grunt/main.js b/chipper/js/grunt/main.js new file mode 100644 --- /dev/null (date 1712863587489) +++ b/chipper/js/grunt/main.js (date 1712863587489) @@ -0,0 +1,28 @@ +// Copyright 2022, University of Colorado Boulder + +/** + * In order to support development with TypeScript for the grunt tasks, transpile before starting and then point + * to the transpiled version. + * + * Since this is the entry point for TypeScript, it and its dependencies must remain in JavaScript. + * + * @author Sam Reid (PhET Interactive Simulations) + */ + +/* eslint-env node */ +const Transpiler = require( '../../js/common/Transpiler' ); + +const commonJSTranspiler = new Transpiler( { verbose: true, mode: 'commonjs' } ); + +// Transpile the entry points +// TODO: What if TypeScript code imports other repos? See https://github.com/phetsims/chipper/issues/1272 +// TODO: Visit these during a watch process, so this can remain just a "safety net" https://github.com/phetsims/chipper/issues/1272 +commonJSTranspiler.transpileRepo( 'chipper' ); +commonJSTranspiler.transpileRepo( 'phet-core' ); +commonJSTranspiler.transpileRepo( 'perennial-alias' ); +commonJSTranspiler.saveCache(); + +// add a type check + +// use chipper's gruntfile +module.exports = require( '../../dist/commonjs/chipper/js/grunt/Gruntfile.js' ); Index: chipper/js/scripts/myScript.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/scripts/myScript.ts b/chipper/js/scripts/myScript.ts new file mode 100644 --- /dev/null (date 1712864717033) +++ b/chipper/js/scripts/myScript.ts (date 1712864717033) @@ -0,0 +1,7 @@ +// for this deno file, import fs +import * as fs from "node:fs"; + +// Deno.readFileSync( '../chipper/tsconfig/all/tsconfig.json', 'utf-8' ); +// +// const x: number = 7; +// console.log( x ); \ No newline at end of file diff --git a/chipper/js/grunt/lint.js b/chipper/js/grunt/lint.ts rename from chipper/js/grunt/lint.js rename to chipper/js/grunt/lint.ts diff --git a/chipper/js/grunt/modulify.js b/chipper/js/grunt/modulify.ts rename from chipper/js/grunt/modulify.js rename to chipper/js/grunt/modulify.ts

samreid commented 7 months ago

I committed some changes. The new patch is:

```diff Subject: [PATCH] Avoid Namespace work in Node.js, see https://github.com/phetsims/chipper/issues/1272 --- Index: js/common/Transpiler.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/Transpiler.js b/js/common/Transpiler.js --- a/js/common/Transpiler.js (revision d9b77311fcdf4d193ed5332fa5f3793dd44ce9f8) +++ b/js/common/Transpiler.js (date 1712863624954) @@ -11,6 +11,8 @@ * @author Sam Reid (PhET Interactive Simulations) */ +// TODO: Move this to perennial, see https://github.com/phetsims/chipper/issues/1272 + // imports const fs = require( 'fs' ); const path = require( 'path' ); @@ -25,11 +27,12 @@ const _ = require( 'lodash' ); // Cache status is stored in chipper/dist so if you wipe chipper/dist you also wipe the cache -const statusPath = '../chipper/dist/js-cache-status.json'; const root = '..' + path.sep; // Directories in a sim repo that may contain things for transpilation // This is used for a top-down search in the initial transpilation and for filtering relevant files in the watch process +// TODO: Subdirs may be different for commonjs/perennial/chipper, see https://github.com/phetsims/chipper/issues/1272 +// TODO: Add chipper/test chipper/eslint chipper/templates and perennial/test at a minimum, see https://github.com/phetsims/chipper/issues/1272 const subdirs = [ 'js', 'images', 'mipmaps', 'sounds', 'shaders', 'common', 'wgsl', // phet-io-sim-specific has nonstandard directory structure @@ -47,15 +50,19 @@ silent: false, // hide all logging but error reporting, include any specified with verbose repos: [], // {string[]} additional repos to be transpiled (beyond those listed in perennial-alias/data/active-repos) brands: [], // {sting[]} additional brands to visit in the brand repo - minifyWGSL: false + minifyWGSL: false, + mode: 'js' // could be 'js' or 'commonjs' }, options ); // @private - this.verbose = options.verbose; + this.verbose = true; this.silent = options.silent; this.repos = options.repos; this.brands = options.brands; this.minifyWGSL = options.minifyWGSL; + this.mode = options.mode; + + this.statusPath = `../chipper/dist/${this.mode}-cache-status.json`; // Track the status of each repo. Key= repo, value=md5 hash of contents this.status = {}; @@ -71,21 +78,21 @@ } // Make sure a directory exists for the cached status file - fs.mkdirSync( path.dirname( statusPath ), { recursive: true } ); + fs.mkdirSync( path.dirname( this.statusPath ), { recursive: true } ); if ( options.clean ) { !this.silent && console.log( 'cleaning...' ); - fs.writeFileSync( statusPath, JSON.stringify( {}, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( {}, null, 2 ) ); } // Load cached status try { - this.status = JSON.parse( fs.readFileSync( statusPath, 'utf-8' ) ); + this.status = JSON.parse( fs.readFileSync( this.statusPath, 'utf-8' ) ); } catch( e ) { !this.silent && console.log( 'couldn\'t parse status cache, making a clean one' ); this.status = {}; - fs.writeFileSync( statusPath, JSON.stringify( this.status, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( this.status, null, 2 ) ); } // Use the same implementation as getRepoList, but we need to read from perennial-alias since chipper should not @@ -99,7 +106,7 @@ * @returns {string} * @public */ - static getTargetPath( filename ) { + getTargetPath( filename ) { const relativePath = path.relative( root, filename ); const suffix = relativePath.substring( relativePath.lastIndexOf( '.' ) ); @@ -108,7 +115,7 @@ relativePath.endsWith( '.mjs' ); const extension = isMJS ? '.mjs' : '.js'; - return Transpiler.join( root, 'chipper', 'dist', 'js', ...relativePath.split( path.sep ) ).split( suffix ).join( extension ); + return Transpiler.join( root, 'chipper', 'dist', this.mode, ...relativePath.split( path.sep ) ).split( suffix ).join( extension ); } /** @@ -135,7 +142,8 @@ // in every sim repo. This strategy is also used in transpile.js presets: [ '../chipper/node_modules/@babel/preset-typescript', - '../chipper/node_modules/@babel/preset-react' + '../chipper/node_modules/@babel/preset-react', + ...( this.mode === 'js' ? [] : [ [ '../chipper/node_modules/@babel/preset-env', { modules: 'commonjs' } ] ] ) ], sourceMaps: 'inline', @@ -143,6 +151,10 @@ [ '../chipper/node_modules/@babel/plugin-proposal-decorators', { version: '2022-03' } ] ] } ).code; + + // TODO: Generalize this so it can look up the appropriate path for any dependency + js = js.split( `require('winston')` ).join( `require('../../../../../../perennial-alias/node_modules/winston')` ); + console.log('hello world'); } fs.mkdirSync( path.dirname( targetPath ), { recursive: true } ); @@ -151,7 +163,15 @@ // @public static modifiedTimeMilliseconds( file ) { - return fs.statSync( file ).mtime.getTime(); + try { + return fs.statSync( file ).mtime.getTime(); + } + catch( e ) { + + // TODO: Why is this file not found? https://github.com/phetsims/chipper/issues/1272 + console.log( 'file not found: ' + file ); + return -1; + } } // @public. Delete any files in chipper/dist/js that don't have a corresponding file in the source tree @@ -225,21 +245,29 @@ // If the file has changed, transpile and update the cache. We have to choose on the spectrum between safety // and performance. In order to maintain high performance with a low error rate, we only write the transpiled file // if (a) the cache is out of date (b) there is no target file at all or (c) if the target file has been modified. - const targetPath = Transpiler.getTargetPath( filePath ); + const targetPath = this.getTargetPath( filePath ); if ( !this.status[ filePath ] || this.status[ filePath ].sourceMD5 !== hash || !fs.existsSync( targetPath ) || this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) { try { let reason = ''; if ( this.verbose ) { - reason = ( !this.status[ filePath ] ) ? ' (not cached)' : ( this.status[ filePath ].sourceMD5 !== hash ) ? ' (changed)' : ( !fs.existsSync( targetPath ) ) ? ' (no target)' : ( this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) ? ' (target modified)' : '???'; + reason = ( !this.status[ filePath ] ) ? ' (not cached)' : + ( this.status[ filePath ].sourceMD5 !== hash ) ? ' (changed)' : + ( !fs.existsSync( targetPath ) ) ? ' (no target)' : + ( this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) ? ' (target modified)' : + '???'; + + if ( this.status[ filePath ] ) { + console.log( filePath, this.status[ filePath ].targetMilliseconds, Transpiler.modifiedTimeMilliseconds( targetPath ) ); + } } this.transpileFunction( filePath, targetPath, text ); this.status[ filePath ] = { sourceMD5: hash, targetMilliseconds: Transpiler.modifiedTimeMilliseconds( targetPath ) }; - fs.writeFileSync( statusPath, JSON.stringify( this.status, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( this.status, null, 2 ) ); const now = Date.now(); const nowTimeString = new Date( now ).toLocaleTimeString(); @@ -344,7 +372,7 @@ // @private saveCache() { - fs.writeFileSync( statusPath, JSON.stringify( this.status, null, 2 ) ); + fs.writeFileSync( this.statusPath, JSON.stringify( this.status, null, 2 ) ); } // @public @@ -399,7 +427,7 @@ const pathExists = fs.existsSync( filePath ); if ( !pathExists ) { - const targetPath = Transpiler.getTargetPath( filePath ); + const targetPath = this.getTargetPath( filePath ); if ( fs.existsSync( targetPath ) && fs.lstatSync( targetPath ).isFile() ) { fs.unlinkSync( targetPath );

samreid commented 7 months ago

I committed some more parts to simplify the remainder:

```diff Subject: [PATCH] Add comments and an unused function, see https://github.com/phetsims/chipper/issues/1272 --- Index: js/common/Transpiler.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/Transpiler.js b/js/common/Transpiler.js --- a/js/common/Transpiler.js (revision 7f24a18186e8b858cde049107b630d4fb7c4df90) +++ b/js/common/Transpiler.js (date 1712898649726) @@ -2,7 +2,9 @@ /** * Transpiles *.ts and copies all *.js files to chipper/dist. Does not do type checking. Filters based on - * perennial-alias/active-repos and subsets of directories within repos (such as js/, images/, and sounds/) + * perennial-alias/active-repos and subsets of directories within repos (such as js/, images/, and sounds/). + * + * Outputs to chipper/dist/js (import statements) and chipper/dist/commonjs (require statements). * * Additionally, will transpile *.wgsl files to *.js files. * @@ -110,20 +112,22 @@ /** * Returns the path in chipper/dist that corresponds to a source file. * @param filename + * @param mode - 'js' or 'commonjs' * @returns {string} * @private */ - static getTargetPath( filename ) { + static getTargetPath( filename, mode ) { + assert( mode === 'js' || mode === 'commonjs', 'invalid mode: ' + mode ); const relativePath = path.relative( root, filename ); const suffix = relativePath.substring( relativePath.lastIndexOf( '.' ) ); // Note: When we upgrade to Node 16, this may no longer be necessary, see https://github.com/phetsims/chipper/issues/1272#issuecomment-1222574593 - // TODO: Get rid of mjs: https://github.com/phetsims/chipper/issues/1272 + // TODO: Get rid of mjs?: https://github.com/phetsims/chipper/issues/1272 const isMJS = relativePath.includes( 'phet-build-script' ) || relativePath.endsWith( '.mjs' ); const extension = isMJS ? '.mjs' : '.js'; - return Transpiler.join( root, 'chipper', 'dist', 'js', ...relativePath.split( path.sep ) ).split( suffix ).join( extension ); + return Transpiler.join( root, 'chipper', 'dist', mode, ...relativePath.split( path.sep ) ).split( suffix ).join( extension ); } /** @@ -131,9 +135,11 @@ * @param {string} sourceFile * @param {string} targetPath * @param {string} text - file text + * @param {string} mode - 'js' or 'commonjs' * @private */ - transpileFunction( sourceFile, targetPath, text ) { + transpileFunction( sourceFile, targetPath, text, mode ) { + assert( mode === 'js' || mode === 'commonjs', 'invalid mode: ' + mode ); let js; if ( sourceFile.endsWith( '.wgsl' ) ) { const pathToRoot = '../'.repeat( sourceFile.match( /\//g ).length - 1 ); @@ -150,7 +156,8 @@ // in every sim repo. This strategy is also used in transpile.js presets: [ '../chipper/node_modules/@babel/preset-typescript', - '../chipper/node_modules/@babel/preset-react' + '../chipper/node_modules/@babel/preset-react', + ...( mode === 'js' ? [] : [ [ '../chipper/node_modules/@babel/preset-env', { modules: 'commonjs' } ] ] ) ], sourceMaps: 'inline', @@ -158,6 +165,10 @@ [ '../chipper/node_modules/@babel/plugin-proposal-decorators', { version: '2022-03' } ] ] } ).code; + + // TODO: Generalize this so it can look up the appropriate path for any dependency, see https://github.com/phetsims/chipper/issues/1272 + // TODO: Note aqua, perennial, perennial-alias, rosetta and skiffle each require winston, see https://github.com/phetsims/chipper/issues/1272 + js = js.split( 'require(\'winston\')' ).join( 'require(\'../../../../../../perennial-alias/node_modules/winston\')' ); } fs.mkdirSync( path.dirname( targetPath ), { recursive: true } ); @@ -166,14 +177,23 @@ // @public static modifiedTimeMilliseconds( file ) { - return fs.statSync( file ).mtime.getTime(); + try { + return fs.statSync( file ).mtime.getTime(); + } + catch( e ) { + + // TODO: Why is this file not found? https://github.com/phetsims/chipper/issues/1272 + console.log( 'file not found: ' + file ); + return -1; + } } // @public. Delete any files in chipper/dist/js that don't have a corresponding file in the source tree - pruneStaleDistFiles() { + pruneStaleDistFiles( mode ) { + assert( mode === 'js' || mode === 'commonjs', 'invalid mode: ' + mode ); const startTime = Date.now(); - const start = '../chipper/dist/js/'; + const start = `../chipper/dist/${mode}/`; const visitFile = path => { path = Transpiler.forwardSlashify( path ); @@ -215,7 +235,7 @@ const endTime = Date.now(); const elapsed = endTime - startTime; - console.log( 'Clean stale chipper/dist/js files finished in ' + elapsed + 'ms' ); + console.log( `Clean stale chipper/dist/${mode} files finished in ` + elapsed + 'ms' ); } // @public join and normalize the paths (forward slashes for ease of search and readability) @@ -224,12 +244,12 @@ } /** - * For *.ts and *.js files, checks if they have changed file contents since last transpile. If so, the - * file is transpiled. - * @param {string} filePath + * @param filePath + * @param mode * @private */ - visitFile( filePath ) { + visitFileWithMode( filePath, mode ) { + assert( mode === 'js' || mode === 'commonjs', 'invalid mode: ' + mode ); if ( _.some( [ '.js', '.ts', '.tsx', '.wgsl', '.mjs' ], extension => filePath.endsWith( extension ) ) && !this.isPathIgnored( filePath ) ) { @@ -240,25 +260,28 @@ // If the file has changed, transpile and update the cache. We have to choose on the spectrum between safety // and performance. In order to maintain high performance with a low error rate, we only write the transpiled file // if (a) the cache is out of date (b) there is no target file at all or (c) if the target file has been modified. - const targetPath = Transpiler.getTargetPath( filePath ); + const targetPath = Transpiler.getTargetPath( filePath, mode ); - if ( !this.status[ filePath ] || this.status[ filePath ].sourceMD5 !== hash || !fs.existsSync( targetPath ) || this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) { + const statusKey = mode === 'js' ? filePath : filePath + '-commonjs'; + + if ( !this.status[ statusKey ] || this.status[ statusKey ].sourceMD5 !== hash || !fs.existsSync( targetPath ) || this.status[ statusKey ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) { try { let reason = ''; if ( this.verbose ) { - reason = ( !this.status[ filePath ] ) ? ' (not cached)' : ( this.status[ filePath ].sourceMD5 !== hash ) ? ' (changed)' : ( !fs.existsSync( targetPath ) ) ? ' (no target)' : ( this.status[ filePath ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) ? ' (target modified)' : '???'; + reason = ( !this.status[ statusKey ] ) ? ' (not cached)' : ( this.status[ statusKey ].sourceMD5 !== hash ) ? ' (changed)' : ( !fs.existsSync( targetPath ) ) ? ' (no target)' : ( this.status[ statusKey ].targetMilliseconds !== Transpiler.modifiedTimeMilliseconds( targetPath ) ) ? ' (target modified)' : '???'; } - this.transpileFunction( filePath, targetPath, text ); + this.transpileFunction( filePath, targetPath, text, mode ); - this.status[ filePath ] = { - sourceMD5: hash, targetMilliseconds: Transpiler.modifiedTimeMilliseconds( targetPath ) + this.status[ statusKey ] = { + sourceMD5: hash, + targetMilliseconds: Transpiler.modifiedTimeMilliseconds( targetPath ) }; fs.writeFileSync( statusPath, JSON.stringify( this.status, null, 2 ) ); const now = Date.now(); const nowTimeString = new Date( now ).toLocaleTimeString(); - !this.silent && console.log( `${nowTimeString}, ${( now - changeDetectedTime )} ms: ${filePath}${reason}` ); + !this.silent && console.log( `${nowTimeString}, ${( now - changeDetectedTime )} ms: ${filePath} ${mode}${reason}` ); } catch( e ) { console.log( e ); @@ -268,17 +291,39 @@ } } + /** + * For *.ts and *.js files, checks if they have changed file contents since last transpile. If so, the + * file is transpiled. + * @param {string} filePath + * @param {string[]} modes - some of 'js','commonjs' + * @private + */ + visitFile( filePath, modes ) { + assert( Array.isArray( modes ), 'invalid modes: ' + modes ); + modes.forEach( mode => { + this.visitFileWithMode( filePath, mode ); + } ); + } + // @private - Recursively visit a directory for files to transpile - visitDirectory( dir ) { + visitDirectory( dir, modes ) { + assert( Array.isArray( modes ), 'invalid modes: ' + modes ); if ( fs.existsSync( dir ) ) { const files = fs.readdirSync( dir ); files.forEach( file => { const child = Transpiler.join( dir, file ); + + // fail out if we visit anything named '/dist' + if ( child.includes( '/dist' ) ) { + console.log( 'uh-oh dist' ); + return; + } + if ( fs.lstatSync( child ).isDirectory() ) { - this.visitDirectory( child ); + this.visitDirectory( child, modes ); } else { - this.visitFile( child ); + this.visitFile( child, modes ); } } ); } @@ -331,24 +376,24 @@ // @public - Visit all the subdirectories in a repo that need transpilation transpileRepo( repo ) { - subdirs.forEach( subdir => this.visitDirectory( Transpiler.join( '..', repo, subdir ) ) ); + subdirs.forEach( subdir => this.visitDirectory( Transpiler.join( '..', repo, subdir ), getModesForRepo( repo ) ) ); if ( repo === 'sherpa' ) { // Our sims load this as a module rather than a preload, so we must transpile it - this.visitFile( Transpiler.join( '..', repo, 'lib', 'game-up-camera-1.0.0.js' ) ); - this.visitFile( Transpiler.join( '..', repo, 'lib', 'pako-2.0.3.min.js' ) ); // used for phet-io-wrappers tests - this.visitFile( Transpiler.join( '..', repo, 'lib', 'big-6.2.1.mjs' ) ); // for consistent, cross-browser number operations (thanks javascript) + this.visitFile( Transpiler.join( '..', repo, 'lib', 'game-up-camera-1.0.0.js' ), getModesForRepo( repo ) ); + this.visitFile( Transpiler.join( '..', repo, 'lib', 'pako-2.0.3.min.js' ), getModesForRepo( repo ) ); // used for phet-io-wrappers tests + this.visitFile( Transpiler.join( '..', repo, 'lib', 'big-6.2.1.mjs' ), getModesForRepo( repo ) ); // for consistent, cross-browser number operations (thanks javascript) Object.keys( webpackGlobalLibraries ).forEach( key => { const libraryFilePath = webpackGlobalLibraries[ key ]; - this.visitFile( Transpiler.join( '..', ...libraryFilePath.split( '/' ) ) ); + this.visitFile( Transpiler.join( '..', ...libraryFilePath.split( '/' ) ), getModesForRepo( repo ) ); } ); } else if ( repo === 'brand' ) { - this.visitDirectory( Transpiler.join( '..', repo, 'phet' ) ); - this.visitDirectory( Transpiler.join( '..', repo, 'phet-io' ) ); - this.visitDirectory( Transpiler.join( '..', repo, 'adapted-from-phet' ) ); + this.visitDirectory( Transpiler.join( '..', repo, 'phet' ), getModesForRepo( repo ) ); + this.visitDirectory( Transpiler.join( '..', repo, 'phet-io' ), getModesForRepo( repo ) ); + this.visitDirectory( Transpiler.join( '..', repo, 'adapted-from-phet' ), getModesForRepo( repo ) ); - this.brands.forEach( brand => this.visitDirectory( Transpiler.join( '..', repo, brand ) ) ); + this.brands.forEach( brand => this.visitDirectory( Transpiler.join( '..', repo, brand ), getModesForRepo( repo ) ) ); } } @@ -414,17 +459,25 @@ const pathExists = fs.existsSync( filePath ); if ( !pathExists ) { - const targetPath = Transpiler.getTargetPath( filePath ); - if ( fs.existsSync( targetPath ) && fs.lstatSync( targetPath ).isFile() ) { - fs.unlinkSync( targetPath ); + + const modes = [ 'js', 'commonjs' ]; + + modes.forEach( mode => { + const targetPath = Transpiler.getTargetPath( filePath, mode ); + if ( fs.existsSync( targetPath ) && fs.lstatSync( targetPath ).isFile() ) { + fs.unlinkSync( targetPath ); - delete this.status[ filePath ]; - this.saveCache(); - const now = Date.now(); - const reason = ' (deleted)'; + // TODO: https://github.com/phetsims/chipper/issues/1272 should the status key always have a suffix? I think so but we will have to let devs know this invalidates the cache. See the reference above as well for statusKey. + const statusKey = mode === 'js' ? filePath : filePath + '-commonjs'; + + delete this.status[ statusKey ]; + this.saveCache(); + const now = Date.now(); + const reason = ' (deleted)'; - !this.silent && console.log( `${new Date( now ).toLocaleTimeString()}, ${( now - changeDetectedTime )} ms: ${filePath}${reason}` ); - } + !this.silent && console.log( `${new Date( now ).toLocaleTimeString()}, ${( now - changeDetectedTime )} ms: ${filePath}${mode}${reason}` ); + } + } ); return; } @@ -443,9 +496,10 @@ } else { const terms = filename.split( path.sep ); - if ( ( this.activeRepos.includes( terms[ 0 ] ) || this.repos.includes( terms[ 0 ] ) ) + const myRepo = terms[ 0 ]; + if ( ( this.activeRepos.includes( myRepo ) || this.repos.includes( myRepo ) ) && subdirs.includes( terms[ 1 ] ) && pathExists ) { - this.visitFile( filePath ); + this.visitFile( filePath, getModesForRepo( myRepo ) ); } } } ); Index: js/grunt/gruntMain.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/grunt/gruntMain.js b/js/grunt/gruntMain.js --- a/js/grunt/gruntMain.js (revision 7f24a18186e8b858cde049107b630d4fb7c4df90) +++ b/js/grunt/gruntMain.js (date 1712899157684) @@ -20,7 +20,8 @@ const Transpiler = require( '../../js/common/Transpiler' ); - const commonJSTranspiler = new Transpiler( { verbose: true, mode: 'commonjs' } ); + // TODO: Remove verbose:true https://github.com/phetsims/chipper/issues/1272 + const commonJSTranspiler = new Transpiler( { verbose: true } ); // Transpile the entry points // If we forgot to transpile something, we will get a module not found runtime error, and @@ -31,6 +32,7 @@ // If there are no changes or a watch process already transpiled the files, this will be a no-op. // Note that 2 Transpile processes trying to write the same file at the same time may corrupt the file, since // we do not have atomic writes. + // TODO: Specify that we only want to output mode=commonjs? https://github.com/phetsims/chipper/issues/1272 commonJSTranspiler.transpileRepo( 'chipper' ); commonJSTranspiler.transpileRepo( 'phet-core' ); commonJSTranspiler.transpileRepo( 'perennial-alias' );

samreid commented 7 months ago

Next steps:

[x] Apply the patch above
[x] Address some of the TODOs, such as standardizing the status cache keys
[x] Notify the dev team that changes are incoming
[x] Commit and push to main
[x] Test on Windows
[x] Notify team members that the transpiler is now outputting a commonjs/requirejs format for some repos. Notify that cache uses different keys and will be invalid. Recommend restarting transpiler with --clean. Notify to be on the lookout for gruntMain issues.
[x] Point Gruntfile entry points at gruntMain
[ ] test a chains build/deploy process
[ ] toggle gruntMain to use typescript
[ ] port 1-2 grunt files to typescript minimally
[ ] move gruntMain to perennial-alias
[ ] move perennial to use perennial/gruntMain instead of js/grunt/Gruntfile.js
[ ] port 1-2 non-grunt files minimally, and consider how we should launch it.
[ ] sourcemaps? how will stack traces look?
[ ] more complete porting of grunt and non-grunt files in chipper
[ ] porting some files in perennial and perennial alias

samreid commented 7 months ago

I swamped this issue with commits, so will continue in #1437. Closing.