Investigate using TypeScript in our build tools and Node code. #1272
Comments
|
From #1206
|
|
@liam-mulhall said:
|
|
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: |
|
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 In #990, all devs confirmed they are using node 14+, which supports See related issue #861 (comment) UPDATE: 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:
For these costs:
Anyways, here's a working prototype: 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
|
|
Here's a simpler change set that accomplishes the same thing. 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
|
|
From today's dev meeting: SR: OK if we start allowing *.ts in chipper? Context is #1272 Grunt would still work but for scripts that use *.ts you would have to launch from node chipper/dist/js/chipper/myscript.js |
|
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:
|
|
To me, it would seem reasonable that chipper could use code from phet-core (which doesn't have any external dependencies).
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 Prototype so far: 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? |
|
@marlitas and I reviewed the proposal above, and it seems promising. We would like to test on windows before committing it to master. |
|
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. |
|
I committed the patch above. I also realized this means the transpiler process needs to be running for
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. |
|
Lots of red in CT from the commits above: Bayes node version is: v14.15.0, my local version is: v16.13.1. That may explain the difference? |
|
@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 @marlitas and I would like to try the *.mjs strategy first. |
|
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 |
|
In experimenting with chipping away at this, I've considered that:
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:
the implementation of the build lifecycle commands
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 The main advantages of this proposal:
The main costs of this approach:
|
|
I swamped this issue with commits, so will continue in #1437. Closing. |
Liam also noted this other one: https://bun.sh/
The text was updated successfully, but these errors were encountered: