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

ci(github): fix type exports in packages/cactus-verifier-client #3654

Closed
wants to merge 1 commit into from

Conversation

ruzell22
Copy link
Contributor

@ruzell22 ruzell22 commented Nov 26, 2024

Commit to be reviewed


ci(github): fix type exports in packages/cactus-verifier-client

Primary Changes
---------------
1. Removed packages/cactus-verifier-client/hyperledger-cactus-verifier-
client-*.tgz in ignore paths in get-all-tgz-path.ts file
2. Removed the export of IVerifierEventListener and LedgerEvent from
./verifier and changed it to source from @hyperledger/cactus-core-api to
resolve the type export issue encountered in this package

Fixes: #3633

Pull Request Requirements

  • Rebased onto upstream/main branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.
  • Have git sign off at the end of commit message to avoid being marked red. You can add -s flag when using git commit command. You may refer to this link for more information.
  • Follow the Commit Linting specification. You may refer to this link for more information.

Character Limit

  • Pull Request Title and Commit Subject must not exceed 72 characters (including spaces and special characters).
  • Commit Message per line must not exceed 80 characters (including spaces and special characters).

A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.

Primary Changes
---------------
1. Removed packages/cactus-verifier-client/hyperledger-cactus-verifier-
client-*.tgz in ignore paths in get-all-tgz-path.ts file
2. Removed the export of IVerifierEventListener and LedgerEvent from
./verifier and changed it to source from @hyperledger/cactus-core-api to
resolve the type export issue encountered in this package

Fixes: hyperledger-cacti#3633

Signed-off-by: ruzell22 <[email protected]>
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@ruzell22 Unfortunately if we remove the VerifierEventListener, LedgerEvent, types from the public API of the verifier-client package then that would be a breaking change and that can only happen when we release v3.0.0.

Before we continue the conversation, could you please double check if there are any other ways to fix the type exports that doesn't involve us not re-exporting the mentioned types from the core-api package?

It's not ideal but we must abide by the conventions to make sure people can safely upgrade minor versions such as from 2.0.0 to 2.0.1 or 2.1.0 when the time comes.

If you can't find any other workaround to make the type exports look good and also doesn't involve breaking changes then just respond back with a list of things/ideas/approaches that you have tried and I'll take a look at it as well. We can decide on how to proceed once we have more data about whether this is possible at all or not.

@ruzell22
Copy link
Contributor Author

@petermetz here are different ways that I did in index.ts back then and now tried it to public-api.ts but it is still not fixing the type exports error.

Test 1:
export { IVerifierEventListener, LedgerEvent } from "./verifier";

Test 2:
import { IVerifierEventListener, LedgerEvent } from "./verifier";
export { IVerifierEventListener, LedgerEvent };

Test 3:
export { IVerifierEventListener, LedgerEvent } from "@hyperledger/cactus-core-api";

Test 4:
export { Verifier, IVerifierEventListener, LedgerEvent } from "./verifier";

etc....

I ran yarn run configure for each test and it reflects the exports in .d.ts file but not on the .js file so maybe that is one finding that can help. On one test (Test 4), I have tried adding Verifier to see if it will reflect in .d.ts and .js and it did, but the two exports IVerifierEventListener, LedgerEvent is not reflecting.

I also tried tweaking the compilerOptions in tsconfig.json but did not do any changes.

@petermetz petermetz added the Breaking_V2 Changes that can only be made with the release of v3.0.0 due to them being breaking changes. label Dec 1, 2024
@petermetz petermetz added this to the v3.0.0 milestone Dec 1, 2024
@petermetz
Copy link
Contributor

@ruzell22 OK, thank you for going the extra mile with the investigations. In that case unfortunately our only resort is to close this PR, temporarily until release v3.0.0 is due for issuance next Spring.
I've created an epic here to collect links to breaking changes we need to make before v3.0.0 and there is also a label applied.
Sorry that we couldn't merge this just now but we must not make breaking changes.
I'm approving and closing it now and we can re-open + merge it as-is when the time comes next year.

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

LGTM

@petermetz
Copy link
Contributor

Temporarily closing, please see above for explanation.

@petermetz petermetz closed this Dec 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking_V2 Changes that can only be made with the release of v3.0.0 due to them being breaking changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ci(github): fix type exports in packages/cactus-verifier-client
2 participants