Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate d.ts files #1424

Closed
1 task done
samreid opened this issue Mar 15, 2024 · 11 comments
Closed
1 task done

Generate d.ts files #1424

samreid opened this issue Mar 15, 2024 · 11 comments

Comments

@samreid
Copy link
Member

samreid commented Mar 15, 2024

From #1356 and #1415, we discussed creating d.ts files.

@jonathanolson indicated d.ts files are the idiomatic way to provide type declarations to open source clients, and we should do so for the scenery-stack.

In experimenting with generating d.ts files in #1356 (comment) we saw that we get around 15,000 type errors since some features we use for mixins are not supported for d.ts files. We saw a conglomerate issue in microsoft/TypeScript#35822 indicating this and other features not present in d.ts files.

In investigation with @matthew-blackman @jonathanolson and @zepumph, we concluded that the way we use mixins is not compatible with generating d.ts files, so we would like to explore alternatives to mixins as we currently have them. One way would be to "copy/paste" instead.

@jonathanolson said he would like to brainstorm some ways to do so.

UPDATE: after getting d.ts files, please un-hold the following issue and see if it is helped by d.ts files:

@samreid
Copy link
Member Author

samreid commented Mar 15, 2024

Notes from the Google doc developer meeting minutes:

Friday Mar 15, 2024

Mini meeting regarding issue 1356 and issue 1415

  • JO: d.ts files are the idiomatic way to support type checking for open source users
  • MK: I agree pushing in this direction will help with modularity and help break circularities

Discussion regarding converting to d.ts files:

  • Make everything public in Node.ts and all mixins and any file that gets mixed into. We don’t like this plan.
  • Copy paste with spam?
  • Eliminate some mixins where possible?
  • Create our own type aliases and use type assertions to make it work.

After investigation:

  • We can get rid of mixin class expressions Or We can get rid of protected members in.

JO: We can’t get d.ts files to work and have mixins anything like we have right now. No!?!?! Here are the problems:

  1. Anonymous class expression, when declared in a d.ts file, lose the knowledge of their supertype extension.
  2. Protected and private members break structural typing when casting variables as their supertype, like const x: Node = new Panel() (broken when panel extends a mixin, because Node has protected members).
  3. Omitting private/protected members from the parent class generally will be a hard problem to solve (our investigation only did this manually).

Consensus: JO MK SR do not see a path forward to use d.ts files with mixins. Our mixin definition is a function that takes a class, and returns another class in which the parameter is the supertype.

To quote JO, “we are dead in the water with mixins.”

SR: What about another language layer on top of typescript to get this to work? Potentially worth investigation, but sounds hard, given IDE support, our current code base and workflows, size of team, etc.

OK. so we can’t use our current mixins with current typescript + incremental mode (declaring d.ts files). Potentially actionables:

About d.ts files:

  1. Wait and see if we will ever be able to generate d.ts with mixins. TypeScript issue 35822 shows many in the TypeScript community that hate this. Here are some other issues:
  2. Remove all mixins. Lots of work, sounds horrible, but JO is optimistic.
    1. Copy pasting mixins for a better type checking API.
    2. More brainstorming.
    3. Do not convert to d.ts incremental type checking.

Without using d.ts files. There is still the out of memory error when type checking from chipper/tsconfig/all/tsconfig.json

  1. We can be more intelligent about providing eslint the minimal tsconfig file, see patch in issue comment
  2. We can tell everyone (and servers) to run Nodejs with more memory. Thus, no more out of memory errors. (ew)
  3. When we want to type check the entire project, currently we use tsc with the all tsconfig, but instead, we could write a script to batch this, so that not all entrypoints are run in the same process (and all info held in memory together). See discussion in issue comment

Timing:

This iteration?

  • JO: I want to brainstorm about what to do with mixins
  • MK: I should probably work on buoyancy
  • SR: I have some time to devote this iteration to the lint entry points and discussing/consulting/brainstorming d.ts

@samreid
Copy link
Member Author

samreid commented Mar 15, 2024

@zepumph
Copy link
Member

zepumph commented Mar 25, 2024

This issue is blocked by phetsims/tasks#1132. Let's go over there to investigate mixins before continuing with d.ts file generation.

@samreid
Copy link
Member Author

samreid commented Apr 30, 2024

  • "outDir": "../chipper/dist/tsc/outDir", /* Redirect output structure to the directory. */

@zepumph
Copy link
Member

zepumph commented Apr 30, 2024

@zepumph
Copy link
Member

zepumph commented Apr 30, 2024

Alright. @samreid and I did lots of investigation about how best to try to keep imported third party javascript files in sherpa/lib from being parsed for d.ts file generation.

Bad ideas we won't do:

  • set allowJs:false, so it doesn't try to parse javascript from imports (2000 lint errors because our typescript code uses 500 javascript files through imports). Booooo,
  • set allowJs:true, checkJs:true. @samreid reported 16840 errors in 1250 files. BOOOOO
  • Use baseURL and paths to do a better job of recognizing sherpa as a third party library instead of our code. This could potentially have worked, but would required changes to Transpiler to run. Perhaps good future work to tighten down how we do our module resolution for code that isn't ours! (but not in this issue).

Instead, all we need is a d.ts file that has the same name as the js file right next to the source in sherpa/lib. Commits inbound.

We also realized that we don't need to bundle project reference conversion ("composite:true") in with d.ts generation ("declaration:true"). We can have two separate steps.

Here is the patch to start for next time:

Subject: [PATCH] patch
---
Index: tsconfig-core.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/tsconfig-core.json b/tsconfig-core.json
--- a/tsconfig-core.json	(revision 980421d810ebbcfd03c0a658549eee7612c6246b)
+++ b/tsconfig-core.json	(date 1714518530689)
@@ -14,7 +14,7 @@
     "allowJs": true,                               /* Allow javascript files to be compiled. */
     "checkJs": false,                              /* Report errors in .js files. */
     "jsx": "react",                                /* Specify JSX code generation: 'preserve', 'react-native', 'react', 'react-jsx' or 'react-jsxdev'. */
-    "declaration": false,    /* Generates corresponding '.d.ts' file. */
+    "declaration": true,    /* Generates corresponding '.d.ts' file. */
     // "declarationMap": true,                      /* Generates a sourcemap for each corresponding '.d.ts' file. */
     "sourceMap": false,                             /* Generates corresponding '.map' file. */
     "outDir": "../chipper/dist/tsc/outDir",         /* Redirect output structure to the directory. */
@@ -23,8 +23,8 @@
     "composite": false,                             /* Enable project compilation (project references) */
     // "tsBuildInfoFile": "./",                     /* Specify file to store incremental compilation information */
     // "removeComments": true,                      /* Do not emit comments to output. */
-    "noEmit": true,                                /* Emit outputs. */
-//    "emitDeclarationOnly": true,                    /* But not js output, because we use babel ourselves */
+    "noEmit": false,                                /* Emit outputs. */
+    "emitDeclarationOnly": true,                    /* But not js output, because we use babel ourselves */
     // "importHelpers": true,                       /* Import emit helpers from 'tslib'. */
     // "downlevelIteration": true,                  /* Provide full support for iterables in 'for-of', spread, and destructuring when targeting 'ES5' or 'ES3'. */
      "isolatedModules": true,                     /* Transpile each file as a separate module (similar to 'ts.transpileModule'). */

zepumph added a commit to phetsims/sherpa that referenced this issue Apr 30, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
zepumph added a commit that referenced this issue Apr 30, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
zepumph added a commit that referenced this issue Apr 30, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
zepumph added a commit that referenced this issue Apr 30, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
@zepumph zepumph assigned zepumph and samreid and unassigned zepumph Apr 30, 2024
@samreid
Copy link
Member Author

samreid commented May 1, 2024

Nice work @zepumph. I also added a commit to add d.ts stubs for 2 perennial files that were getting bundled in. After that change, with the patch in the preceding comment, our entire project generates d.ts files from tsconfig/all then tsc

Can we commit the patch above and close this issue? I'm not sure what's left to do. Does @jonathanolson want to start leveraging these/publishing these in scenery stack?

zepumph added a commit that referenced this issue May 1, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
@zepumph
Copy link
Member

zepumph commented May 1, 2024

After discussion with @jonathanolson, @samreid and I are now ready to commit this change. Closing.

@pixelzoom
Copy link
Contributor

pixelzoom commented May 1, 2024

Reopening with top priority. grunt dev is failing:

% cd gas-properties
% grunt dev --brands=phet,phet-io
Running "dev" task
(Forwarding task to perennial)
Running "dev" task
info: getting dependencies.json for gas-properties
info: linting gas-properties
Fatal error: Perennial task failed:
Error: grunt lint-all in ../gas-properties failed with exit code 1
stdout:
Running "lint-all" task
[........................................] 100.00%


/Users/cmalley/PhET/GitHub/perennial-alias/js/common/ChipperVersion.d.ts
  0:0  error  Parsing error: ESLint was configured to run on `/Users/cmalley/PhET/GitHub/perennial-alias/js/common/ChipperVersion.d.ts` using `parserOptions.project`: /users/cmalley/phet/github/chipper/tsconfig/all/tsconfig.json
However, that TSConfig does not include this file. Either:
- Change ESLint's list of included files to not include this file
- Change that TSConfig to include this file
- Create a new TSConfig that includes this file and include it in your parserOptions.project
See the typescript-eslint docs for more info: https://typescript-eslint.io/linting/troubleshooting#i-get-errors-telling-me-eslint-was-configured-to-run--however-that-tsconfig-does-not--none-of-those-tsconfigs-include-this-file

/Users/cmalley/PhET/GitHub/perennial-alias/js/common/ReleaseBranch.d.ts
  0:0  error  Parsing error: ESLint was configured to run on `/Users/cmalley/PhET/GitHub/perennial-alias/js/common/ReleaseBranch.d.ts` using `parserOptions.project`: /users/cmalley/phet/github/chipper/tsconfig/all/tsconfig.json
However, that TSConfig does not include this file. Either:
- Change ESLint's list of included files to not include this file
- Change that TSConfig to include this file
- Create a new TSConfig that includes this file and include it in your parserOptions.project
See the typescript-eslint docs for more info: https://typescript-eslint.io/linting/troubleshooting#i-get-errors-telling-me-eslint-was-configured-to-run--however-that-tsconfig-does-not--none-of-those-tsconfigs-include-this-file

✖ 2 problems (2 errors, 0 warnings)

grunt dev --disable-eslint-cache and grunt dev --brands=phet,phet-io --lint=false were failing with the same errors.

I don't know if 143ff65 is supposed to be a fix, but it didn't work for me after pull.

I was able to deploy with grunt dev --brands=phet,phet-io --lint=false after deleting chipper/eslint/cache.

@pixelzoom pixelzoom reopened this May 1, 2024
@samreid
Copy link
Member Author

samreid commented May 2, 2024

Thanks, I was able to reproduce the problem and will open a separate issue for discussion.

@samreid
Copy link
Member Author

samreid commented May 2, 2024

OK I opened #1440 and propagated the priority flags. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants