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: replace err-code with CodeError #1532

Merged
merged 5 commits into from
Mar 20, 2023

Conversation

tabcat
Copy link
Contributor

@tabcat tabcat commented Jan 5, 2023

Replaces err-code with CodeError

Related: #1269

Changes

  • removes err-code from dependencies
  • adds @libp2p/[email protected] to dependencies
  • uses CodeError in place of err-code

Blockers

@tabcat tabcat changed the title chore: use CodeError chore: replace err-code with CodeError Jan 5, 2023
@tabcat tabcat changed the title chore: replace err-code with CodeError refactor: replace err-code with CodeError Jan 8, 2023
@tabcat
Copy link
Contributor Author

tabcat commented Jan 8, 2023

Two options to handle the .props breaking change wrt err-code -> CodeError

  • refactor CodeError to match err-code
    • generic typing for props would not make sense anymore (bad)
  • upgrade any internal code relying on this
    • if the keychain's cms decrypt function is public or is used by other libp2p/ipfs packages
      • seems like a breaking change?
      • document change and link to upgrade fix from change log

@tabcat tabcat changed the title refactor: replace err-code with CodeError chore: replace err-code with CodeError Jan 10, 2023
@wemeetagain
Copy link
Member

we'll want to go with this option:

upgrade any internal code relying on this

@tabcat
Copy link
Contributor Author

tabcat commented Jan 13, 2023

Ill start looking for those cases then in ipfs/libp2p and other related dependents

@tabcat
Copy link
Contributor Author

tabcat commented Jan 24, 2023

From what I could find from the dependents on npm, this doesnt seem to be used internally. It is documented in the code as an API. Will need to update that.

@tabcat tabcat changed the title chore: replace err-code with CodeError chore!: replace err-code with CodeError Feb 15, 2023
Comment on lines 127 to 129
throw errCode(new Error(`Decryption needs one of the key(s): ${missingKeys.join(', ')}`), codes.ERR_MISSING_KEYS, {
throw new CodeError(`Decryption needs one of the key(s): ${missingKeys.join(', ')}`, codes.ERR_MISSING_KEYS, {
missingKeys
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only piece of code affected by the props change in CodeError across all of the libp2p packages.

* exists, an Error is returned with the property 'missingKeys'. It is array of key ids.
* exists, an Error is returned with the property '.props.missingKeys'. It is array of key ids.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if this needs to be reworded.

@tabcat tabcat marked this pull request as ready for review February 15, 2023 12:40
@p-shahi
Copy link
Member

p-shahi commented Feb 28, 2023

Let's resolve conflicts in this PR after #1533 is merged @tabcat

@p-shahi
Copy link
Member

p-shahi commented Mar 2, 2023

@tabcat this is unblocked now that #1533 has been merged

chore: upgrade @libp2p/interfaces to v3.2.0

chore: remove err-code from deps

replace errCode imports with CodeError

replace errCode implementations with CodeError
@tabcat tabcat changed the title chore!: replace err-code with CodeError chore: replace err-code with CodeError Mar 2, 2023
@tabcat
Copy link
Contributor Author

tabcat commented Mar 2, 2023

Since the keychain has been moved to it's own repo this is no longer a breaking change.

@achingbrain achingbrain changed the title chore: replace err-code with CodeError fix: replace err-code with CodeError Mar 20, 2023
@achingbrain achingbrain merged commit d7fa853 into libp2p:master Mar 20, 2023
@achingbrain
Copy link
Member

Thanks for opening this and having such patience. We got there!

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.

4 participants