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

fix(types): typescript type generation improvements (contributes to #402) #405

Merged
merged 2 commits into from
Mar 25, 2022

Conversation

sstone1
Copy link
Contributor

@sstone1 sstone1 commented Mar 25, 2022

Lots of changes to improve TypeScript generation across all modules:

  • Install TypeScript as a development dependency into all projects
  • Remove npx -p typescript tsc from build:types target and just use tsc instead
  • Force build:types to be run during CI by adding it as the postdoc target (it validates all types are correct)
  • Remove JSON requires that are exported and replace with JSON.parse(fs.readFileSync(...)) equivalent
    These don't work well with TypeScript generation; the compiler omits them altogether 🤦
  • Remove const Thing = require('module').Thing and use const { Thing } = require('module')
    This style of imports doesn't work well with TypeScript generation, especially when multiple imports of the same module occur - it's also redundant to type Thing twice
  • Import types that are used in JSDoc comments but not elsewhere in the code to give TypeScript everything it needs
    These imports are wrapped in eslint-disable and eslint-enable blocks because they are unused in code.
    These imports are wrapped in a if (global === undefined) { (never true) block because otherwise we get circular requires and these cause the code to fall over in a heap
    These imports are wrapped in a istanbul ignore next block because the if condition will never be true
  • Fixed a bunch of @private vs @protected errors which the TypeScript compiler caught
  • Enable TypeScript generation for concerto-tools

@sstone1 sstone1 force-pushed the issue-402 branch 2 times, most recently from 3ec0f66 to ef0b52c Compare March 25, 2022 10:59
@mttrbrts mttrbrts requested review from dselman, jeromesimeon and mttrbrts and removed request for dselman and jeromesimeon March 25, 2022 11:38
Copy link
Member

@mttrbrts mttrbrts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following pattern is incredibly ugly, but I don't see another way around it beyond migrating the whole project to TypeScript.

// Types needed for TypeScript generation.
/* eslint-disable no-unused-vars */
/* istanbul ignore next */
if (global === undefined) {
  ...
}
/* eslint-enable no-unused-vars */

I'd be happy to accept it as a short-term measure to unblock release of v2. Can you open an issue for porting to TypeScript as a long-term solution for v3+?

@sstone1 sstone1 mentioned this pull request Mar 25, 2022
@mttrbrts mttrbrts merged commit 6a58a48 into accordproject:master Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants