Open samreid opened 1 year ago
Project references requires changing "noEmit": true
to "noEmit": false
. When I did that, I also changed to "emitDeclarationOnly": true,
. However, now there are Found 9842 errors in 68 files.
for all our mixins, like:
../../../wave-interference/js/common/view/WaveMeterProbeNode.ts:23:7 - error TS4094: Property 'nodesThatAreActiveDescendantToThisNode' of exported class expression may not be private or protected.
23 class WaveMeterProbeNode extends InteractiveHighlighting( ProbeNode ) {
~~~~~~~~~~~~~~~~~~
../../../wave-interference/js/common/view/WaveMeterProbeNode.ts:23:7 - error TS4094: Property 'onBoundsListenersAddedOrRemoved' of exported class expression may not be private or protected.
23 class WaveMeterProbeNode extends InteractiveHighlighting( ProbeNode ) {
~~~~~~~~~~~~~~~~~~
The workaround described in https://github.com/microsoft/TypeScript/issues/30355#issuecomment-839834550 seems to work OK though, if we are happy maintaining one type per mixin.
In process investigation:
More progress:
We did progress and next need to create types for mixins:
@samreid and I got a bit further with mixins today, but can't seem to work out the hierarchy in voicing right now. In this patch there is good progress, but the next steps are that DelayedMutate's doesn't take into account the parent type at all, so the subclass (Voicing) thinks it doesn't fulfill the needs of being Node-like.
Here are my top 30 tsc times from precommit hooks in December.
I should note I also run tsc
without the precommit hooks, and it is often taking 80+ seconds. This is very unfortunate, and it makes me want to bump the priority of this issue. But even getting to a point where we can test the performance of this approach will be time consuming.
Adding this to DelayedMutateType helps the patch above a lot.
type DelayedMutateType = {
mutate( options?: NodeOptions ): DelayedMutateType;
} & Node;
403 type errors in this patch:
TypeScript 5.0 beta was released today, I wonder if that would simplify anything here? There were some notes about changes for emitDeclaration in tsc --build
but that may just be about command line options. Also, TypeScript 5.0 claims to have speed improvements like 10%-20% which may help.
In https://github.com/phetsims/chipper/issues/1415#issuecomment-1879232186, @samreid and I spent some time understanding just how many entry points we can add to tsconfig/all/tsconfig.json
before we are no longer able to type check because it is too big to fit into memory (heap crash). We believe that it ~30 more entrypoints could be added, but we currently have 60 sims in javascript with no entry points in that file (not to mention new sims always being created starting in typescript).
This should bump the priority of this issue, because project references are the way out of storing the entirety of the codebases type information in memory. We always knew this day would come eventually, but let's raise the priority of this issue well BEFORE we are nearing our 30th new entrypoint over there.
The primary reason this is a higher priority is because eslint uses the all config for its type info. Perhaps a workaround would be to have a smarter way to specify the eslint config's project path (maybe custom to the lint task being done?).
We'd like to bring this up to dev meeting to add to the proposed planning topics, mostly to get this on peoples radar.
@marlitas indicated that @samreid and @matthew-blackman will likely be working on this in upcoming iterations.
I tested the hypothesis that if we put all of the mixins in one bundle, then they can still infer (from the dynamic part) properly. However, with this patch:
It goes from 48 errors to 10541 errors when it crosses the threshold and starts checking for properties of exported class expressions not being private or protected:
../../../sun/js/ToggleSwitch.ts:86:22 - error TS4094: Property 'speakContent' of exported class expression may not be private or protected.
86 export default class ToggleSwitch<T> extends Voicing( Node ) {
~~~~~~~~~~~~
../../../sun/js/ToggleSwitch.ts:86:22 - error TS4094: Property 'updateActiveDescendantAssociationsInPeers' of exported class expression may not be private or protected.
86 export default class ToggleSwitch<T> extends Voicing( Node ) {
~~~~~~~~~~~~
../../../sun/js/ToggleSwitch.ts:86:22 - error TS4094: Property 'updateMaxDimension' of exported class expression may not be private or protected.
86 export default class ToggleSwitch<T> extends Voicing( Node ) {
~~~~~~~~~~~~
../../../sun/js/ToggleSwitch.ts:86:22 - error TS4094: Property 'updateSelfBounds' of exported class expression may not be private or protected.
86 export default class ToggleSwitch<T> extends Voicing( Node ) {
~~~~~~~~~~~~
../../../tandem/js/Tandem.ts:490:26 - error TS4094: Property 'dispose' of exported class expression may not be private or protected.
490 public static readonly ROOT = new Tandem.RootTandem( null, _.camelCase( packageJSON.name ) );
~~~~
../../../tandem/js/Tandem.ts:490:26 - error TS4094: Property 'isDisposed' of exported class expression may not be private or protected.
490 public static readonly ROOT = new Tandem.RootTandem( null, _.camelCase( packageJSON.name ) );
~~~~
../../../tandem/js/Tandem.ts:490:26 - error TS4094: Property 'removeChild' of exported class expression may not be private or protected.
490 public static readonly ROOT = new Tandem.RootTandem( null, _.camelCase( packageJSON.name ) );
~~~~
Found 10541 errors.
Would abandoning mixins be one way forward here?
See notes in https://github.com/TerriaJS/terriajs/issues/5866#issuecomment-1078585855
typeof
emits were decided against in https://github.com/microsoft/TypeScript/issues/41824#issue-757463042
Experimenting with mixins, I created this sample project:
type Constructor<T = object, K extends any[] = any[]> = new ( ...args: K ) => T;
const Popupable = <SuperType extends Constructor<Node>>( type: SuperType, optionsArgPosition: number ) => { // eslint-disable-line @typescript-eslint/explicit-module-boundary-types
return class extends type {
protected readonly layoutBounds = null;
/**
* Hide the popup. If you create a new popup next time you show(), be sure to dispose this popup instead.
*/
public hide(): void {
}
protected show(): void {
}
};
};
export default Popupable;
/**
* Shared defaults for tsconfig files.
* @author Sam Reid (PhET Interactive Simulations)
*/
{
"compilerOptions": {
/* Visit https://aka.ms/tsconfig.json to read more about this file */
/* Basic Options */
"incremental": true, /* Enable incremental compilation */
"target": "ES2020", /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', 'ES2018', 'ES2019', 'ES2020', 'ES2021', or 'ESNEXT'. */
"module": "ES2020", /* Specify module code generation: 'none', 'commonjs', 'amd', 'system', 'umd', 'es2015', 'es2020', or 'ESNext'. */
"allowJs": true, /* Allow javascript files to be compiled. */
"checkJs": false, /* Report errors in .js files. */
"declaration": false, /* Generates corresponding '.d.ts' file. */
"sourceMap": false, /* Generates corresponding '.map' file. */
"outDir": "./outdir", /* Redirect output structure to the directory. */
"rootDir": "../", /* Specify the root directory of input files. Use to control the output directory structure with --outDir. */
// "rootDir": "./", /* Specify the root directory of input files. Use to control the output directory structure with --outDir. */
"composite": false, /* Enable project compilation */
// "noEmit": true, /* Do not emit outputs. */
// "emitDeclarationOnly": true,
"isolatedModules": true, /* Transpile each file as a separate module (similar to 'ts.transpileModule'). */
"strict": true, /* Enable all strict type-checking options. */
"noImplicitReturns": true, /* Report error when not all code paths in function return a value. */
"noImplicitOverride": true, /* Ensure overriding members in derived classes are marked with an 'override' modifier. */
"allowUmdGlobalAccess": true, /* Allow accessing UMD globals from modules. */
"skipLibCheck": true, /* Skip type checking of declaration files. */
"forceConsistentCasingInFileNames": true /* Disallow inconsistently-cased references to the same file. */
},
"include": [
"js/**/*.ts"
]
}
This type checks correctly from the command line:
~/phet/root/tinyproj$ tsc -b
~/phet/root/tinyproj$
However, changing the following (necessary for a project reference):
Gives these type checking errors:
~/phet/root/tinyproj$ tsc -b
js/Popupable.ts:3:7 - error TS4094: Property 'layoutBounds' of exported class expression may not be private or protected.
3 const Popupable = <SuperType extends Constructor<Node>>( type: SuperType, optionsArgPosition: number ) => { // eslint-disable-line @typescript-eslint/explicit-module-boundary-types
~~~~~~~~~
js/Popupable.ts:3:7 - error TS4094: Property 'show' of exported class expression may not be private or protected.
3 const Popupable = <SuperType extends Constructor<Node>>( type: SuperType, optionsArgPosition: number ) => { // eslint-disable-line @typescript-eslint/explicit-module-boundary-types
~~~~~~~~~
Found 2 errors.
~/phet/root/tinyproj$
This means that we cannot have the following two things at the same time:
Note that @pixelzoom said in https://github.com/phetsims/chipper/issues/1267#issuecomment-1154457049
Therefore we have these options:
Changing mixin members to be public works around this problem. However, note that @pixelzoom said in https://github.com/phetsims/chipper/issues/1267#issuecomment-1154457049
Excellent, glad we can add accessibility modifiers to mixins.
So it would be unfortunate if we had to make everything in mixins public.
We could abandon project references, and continue using the "monolithic" strategy for type checking a full stack at once, which avoids emitting mixin private/protected members. We could look into other ways to conserve memory. One way could be to make change the lint configurations to be "per simulation and its dependencies" and get rid of the "everything" lint configuration. This may be the easiest thing to investigate next (lowest up-front cost). But aside from reducing the memory footprint, there could be other advantages of using project references, such as speed/modularity. So it would be unfortunate to rule out memory and time-saving type checking/linting because we cannot use project references. It could also be unfortunate to not be able to type check the entire project with one tsc
invocation.
It seems we have around 15 mixins defined across the project. Mixins that only mix into one thing could be converted to a class. For instance, at the moment, Paintable only mixes like Paintable(Node)
, so it could be converted to PaintableNode extends Node. This should not be done lightly since other developers may have plans for mixing things into other types in the future. Other cases like Voicing(VBox)
could be converted to inheritance + composition VoicingNode extends Node{ this.addChild(new VBox())}
. There are reasons other than this to abandon mixins. For instance, WebStorm/IntelliJ cannot currently autocomplete mixin methods:
However, a lot of work went into setting up mixins the way they are and it would be a considerable development effort to convert to an alternate paradigm.
Maybe someone can think of another workaround or alternative to the ideas above. For instance, I haven't investigated what happens if you put a ts-ignore on those errors (I suspect they would not be emitted). But maybe one hack could be to ts-ignore the errors and export a type declaration that does the right thing (but type aliases cannot have protected or private members either). I also briefly experimented with isolatedModules: false
and saw similar problems. Anyways, just leaving a placeholder for "something else" in further brainstorming.
I'm also somewhat confused about the scope of the exported class expressions rule. For instance, some of the 10541 errors in https://github.com/phetsims/chipper/issues/1356#issuecomment-1989558788 are like:
../../../sun/js/ToggleSwitch.ts:86:22 - error TS4094: Property 'updateSelfBounds' of exported class expression may not be private or protected.
86 export default class ToggleSwitch<T> extends Voicing( Node ) {
~~~~~~~~~~~~
Note that updateSelfBounds
is not declared in the mixin. But when I make all of the updateSelfBounds
public like so:
The number of errors goes from 10541 errors to 10477 errors.
Please also note the above is not just a problem with mixins, but also a problem with any exported class expression. For instance, Tandem.ts defines this class expression:
private static readonly RootTandem = class RootTandem extends Tandem {
/**
* RootTandems only accept specifically named children.
*/
public override createTandem( name: string, options?: TandemOptions ): Tandem {
if ( Tandem.VALIDATION ) {
const allowedOnRoot = name === window.phetio.PhetioIDUtils.GLOBAL_COMPONENT_NAME ||
name === REQUIRED_TANDEM_NAME ||
name === OPTIONAL_TANDEM_NAME ||
name === TEST_TANDEM_NAME ||
name === window.phetio.PhetioIDUtils.GENERAL_COMPONENT_NAME ||
_.endsWith( name, Tandem.SCREEN_TANDEM_NAME_SUFFIX );
assert && assert( allowedOnRoot, `tandem name not allowed on root: "${name}"; perhaps try putting it under general or global` );
}
return super.createTandem( name, options );
}
};
Since it has protected/private members, it fails this type checking rule and cannot be emitted for a d.ts file.
I do not know how much of our project has class expressions related to mixins vs other class expressions. Searching = class
yields this result and a few more. For instance: EventTimer.ts has public static readonly PoissonEventModel = class PoissonEventModel {
I wrote in slack:
Marla indicated https://github.com/phetsims/chipper/issues/1356 will be priorized in coming iterations and that I will likely be on the subteam. In order to understand the problem better, I ran an experiment in https://github.com/phetsims/chipper/issues/1356#issuecomment-1993364319 and it looks like class expressions with protected/private members are not compatible with typescript project references. (Does this sound accurate to you?) We use class expressions in a few ways across our project, but they are the foundation for our mixin pattern. So I wanted to ask Jesse, Michael, Chris and Jonathan, should this issue be about adjusting the way we do mixins (for instance: making everything public, or converting from mixins to inheritance + composition), or should the issue treat mixins as a given and look for other ways to reduce the memory footprint (ways other than typescript project references). Having a brief conversation about this now could inform who should be involved on that subteam.
I was surprised to see that this type checked OK:
I visited all mixins and made everything public (in the mixins). This took the number of type errors from 15,000 down to 8,000. But the remaining errors look like this and I cannot explain it:
../../../sun/js/ToggleSwitch.ts:86:22 - error TS4094: Property 'removeNodeThatIsActiveDescendantThisNode' of exported class expression may not be private or protected.
86 export default class ToggleSwitch<T> extends Voicing( Node ) {
~~~~~~~~~~~~
../../../sun/js/ToggleSwitch.ts:86:22 - error TS4094: Property 'setRendererBitmask' of exported class expression may not be private or protected.
86 export default class ToggleSwitch<T> extends Voicing( Node ) {
~~~~~~~~~~~~
../../../sun/js/ToggleSwitch.ts:86:22 - error TS4094: Property 'updateActiveDescendantAssociationsInPeers' of exported class expression may not be private or protected.
86 export default class ToggleSwitch<T> extends Voicing( Node ) {
~~~~~~~~~~~~
../../../sun/js/ToggleSwitch.ts:86:22 - error TS4094: Property 'updateMaxDimension' of exported class expression may not be private or protected.
86 export default class ToggleSwitch<T> extends Voicing( Node ) {
~~~~~~~~~~~~
../../../sun/js/ToggleSwitch.ts:86:22 - error TS4094: Property 'updateSelfBounds' of exported class expression may not be private or protected.
86 export default class ToggleSwitch<T> extends Voicing( Node ) {
~~~~~~~~~~~~
Found 8029 errors.
``
Here is a working strategy that specifies the tsconfig file in the repo's package.json:
As per dev meeting on 2024-03-21:
SR Summary: We had a few related problems:
JO will try to work on this for next iteration
So to be clear, the item that needs prioritization and scheduling that @jonathanolson is the lead on is https://github.com/phetsims/chipper/issues/1424. This issue isn't really the path forward.
We addressed mixins. Here is a patch that enables compilation (project references) in a few repos. build.json is one bundle. It is working really well. Let's review with @jonathanolson and @zepumph and see if we want to propagate this pattern across our repos (probably a script that reads package.json phetLibs and translates into tsconfig.references.
UPDATE:
{
"extends": "../../tsconfig-core.json",
// Explicitly list all entry points that we want to type check.
// Imported images/mipmaps/sounds are still type checked.
// This structure was determined in https://github.com/phetsims/chipper/issues/1245
"references": [
{
"path": "../buildjson"
}
],
"include": [
"../../../acid-base-solutions/js/**/*",
"../../../alpenglow/js/**/*",
"../../../aqua/js/**/*",
"../../../area-model-algebra/js/**/*",
We may still want to use the "all" one when doing lint-everything.
New patch:
tsc
do when you're project is set up with project references.
From https://github.com/phetsims/chipper/issues/1350, there are many referenced issues about type checking in https://github.com/phetsims/chipper/issues/1350#issuecomment-1309022839. We used to use very fine project references and were surprised to see that things sped up when converting to the monolithic approach. But maybe we should investigate a coarse approach, where there is one "module" for common code, and each sim has its own module. That way, when changing one sim, it won't need to type check other sims. But changing common code would still need to type check all sims.
Anyways, we should test performance for this strategy on mac and windows, for common code change and sim change.