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: fix typing issues with typescript 4.9 #1214

Merged
merged 2 commits into from
Jan 17, 2023

Conversation

TimoGlastra
Copy link
Contributor

Updates typescript to 4.9.4, and also updates some related dev dependencies.

Haven't updated any non-dev dependencies (that should be done separately as mentioned in #1213).

Fixes #1205

There were some issues specific to the latest TypeScript release which broke the types of AFJ. Mainly I've made the followign changes:

  • update ts to 4.9.4
  • Add @types/express to dependencies in @aries-framework/node. TS will complain that it can't find the types otherwise.
  • Due to the update of eslint and typescript-eslint the order of imports have changed. Not sure why, but there's no functional change to the code
  • Simplifies and changed the broken types in the credentials api.

@codecov-commenter
Copy link

codecov-commenter commented Jan 16, 2023

Codecov Report

Merging #1214 (a2200a6) into main (c697716) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1214      +/-   ##
==========================================
+ Coverage   87.59%   87.68%   +0.08%     
==========================================
  Files         734      734              
  Lines       17400    17535     +135     
  Branches     2928     2928              
==========================================
+ Hits        15242    15376     +134     
- Misses       2016     2017       +1     
  Partials      142      142              
Impacted Files Coverage Δ
...ages/action-menu/src/services/ActionMenuService.ts 96.59% <ø> (ø)
packages/core/src/agent/Agent.ts 95.79% <ø> (ø)
packages/core/src/agent/AgentConfig.ts 95.65% <ø> (ø)
packages/core/src/agent/AgentModules.ts 100.00% <ø> (ø)
packages/core/src/agent/BaseAgent.ts 95.29% <ø> (ø)
packages/core/src/agent/EnvelopeService.ts 100.00% <ø> (ø)
packages/core/src/agent/Events.ts 100.00% <ø> (ø)
packages/core/src/agent/MessageReceiver.ts 82.30% <ø> (ø)
packages/core/src/agent/MessageSender.ts 86.47% <ø> (ø)
packages/core/src/agent/TransportService.ts 100.00% <ø> (ø)
... and 224 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@genaris
Copy link
Contributor

genaris commented Jan 16, 2023

It seems that type imports in eslint-plugin-import are a relatively recent feature, and they were doing some tweaks on their handling between 2.26.0 (the one we use) and 2.27.4 (the latest one). I don't see a specific reason why now it expects sibling types to be before parent ones, but anyway it is handling 'type' group separately, so it does not take into account if we chose parents to be before siblings or vice versa.

I'm OK with these changes even if they are not completely consistent with the configured rules, but I'm curious about the reasons of separating type imports from the other ones. Is there any standard import ordering convention leveraging that?

@TimoGlastra
Copy link
Contributor Author

but anyway it is handling 'type' group separately, so it does not take into account if we chose parents to be before siblings or vice versa

Yeah this has been the case since we've started using type imports. Not sure why, but it's okay I think. I don't really mind the order, as long as it's consistent so we don't have merge conflicts based on it.

@TimoGlastra
Copy link
Contributor Author

but I'm curious about the reasons of separating type imports from the other ones.

I kinda like it because it shows it's used as a type, and thus will be removed at compilation.

Copy link
Contributor

@morrieinmaas morrieinmaas left a comment

Choose a reason for hiding this comment

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

LGTM! massive PR @TimoGlastra. In general, reading through this I was wondering whether it'd make sense to use interfaces instead of abstract classes for base instances. Not 100% sure, but generally might be nice if the base class(es) never get(s) instantiated. Not part of this PR anyway but just as an idea. With interfaces we'd still get the typing right but without the impl (which saves some space by getting compiled away, given we don't need an impl of the abstract base itself, but just the implementations and extensions thereof).

@TimoGlastra
Copy link
Contributor Author

Makes sense @morrieinmaas, thanks for the feedback. I've recently removed some abstract classes for interfaces and it felt a lot cleaner. Only place where it's useful is abstract classes that implement some functions, but we could also just extract them in util methods and implement twice 👍

Will keep in mind for future PRs

@TimoGlastra TimoGlastra enabled auto-merge (squash) January 17, 2023 15:18
@TimoGlastra TimoGlastra merged commit 087980f into openwallet-foundation:main Jan 17, 2023
TimoGlastra added a commit that referenced this pull request May 16, 2023
Co-authored-by: Karim Stekelenburg <[email protected]>
Co-authored-by: Ariel Gentile <[email protected]>
Co-authored-by: Timo Glastra <[email protected]>
Co-authored-by: Jim Ezesinachi <[email protected]>
Co-authored-by: Ry Jones <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Kim Ebert <[email protected]>
Co-authored-by: Grammatopoulos Athanasios Vasileios <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Victor Anene <[email protected]>
Co-authored-by: Jim Ezesinachi <[email protected]>
Co-authored-by: KolbyRKunz <[email protected]>
Co-authored-by: Berend Sliedrecht <[email protected]>
Co-authored-by: Jason C. Leach <[email protected]>
Co-authored-by: Martin Auer <[email protected]>
Co-authored-by: Niall Shaw <[email protected]>
Co-authored-by: Pritam Singh <[email protected]>
Co-authored-by: Mo <[email protected]>
Co-authored-by: NB-MikeRichardson <[email protected]>
Co-authored-by: Amit-Padmani <[email protected]>
Co-authored-by: DaevMithran <[email protected]>
Co-authored-by: Alexander Shenshin <[email protected]>
fix(openid4vc-client): set package to private (#1210)
fix: fix typing issues with typescript 4.9 (#1214)
Fixes #1205
resolver (#1247)
fix: set updateAt on records when updating a record (#1272)
fix(transport)!: added docs moved connection to connectionId (#1222)
fix(indy-vdr): export relevant packages from root (#1291)
fix(askar): generate nonce suitable for anoncreds (#1295)
resolver and registrar for did:indy (#1253)
fix: imports from core (#1303)
fix: thread id improvements (#1311)
fix: loosen base64 validation (#1312)
fix(samples): dummy module response message type (#1321)
fix: seed and private key validation and return type in registrars (#1324)
fix!: don't emit legacy did:sov prefix for new protocols (#1245)
fix(askar): anoncrypt messages unpacking (#1332)
fix: expose indy pool configs and action menu messages (#1333)
fix: create new socket if socket state is 'closing' (#1337)
fix(anoncreds): include prover_did for legacy indy (#1342)
fix(indy-sdk): import from core (#1346)
fix(anoncreds-rs): save revocation registry index (#1351)
fix: isNewSocket logic (#1355)
fix(tenant): Correctly configure storage for multi tenant agents (#1359)
Fixes hyperledger#1353
fix(anoncreds): Buffer not imported from core (#1367)
fix(core): repository event when calling deleteById (#1356)
fix(askar): custom error handling (#1372)
fix: return HTTP 415 if unsupported content type (#1313)
fix: remove named capture groups (#1378)
fix example usage of indy-sdk-react-native package (#1382)
fix: connection id in sessions for new connections (#1383)
fix: did cache key not being set correctly (#1394)
fix: incorrect type for anoncreds registration (#1396)
fix: reference to indyLedgers in IndyXXXNotConfiguredError (#1397)
fix: add reflect-metadata (#1409)
fix: various anoncreds revocation fixes (#1416)
fix: jsonld credential format identifier version (#1412)
fix: remove `deleteOnFinish` and added documentation (#1418)
fix(askar): default key derivation method (#1420)
fix(anoncreds): make revocation status list inline with the spec (#1421)
fix(anoncreds-rs): revocation status list as JSON (#1422)
fix: issuance with unqualified identifiers (#1431)
fix(connections): store imageUrl when using DIDExchange (#1433)
fix(indy-vdr): do not force indy-vdr version (#1434)
fix: small issues with migration and WAL files (#1443)
fix: migration of link secret (#1444)
fix: Emit RoutingCreated event for mediator routing record (#1445)
fix: small updates to cheqd module and demo (#1439)
fix: remove scope check from response (#1450)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CredentialsApi types does not satisfy the constraints
4 participants