Skip to content
This repository has been archived by the owner on Aug 24, 2021. It is now read-only.

fix: update aegir and fix types #79

Merged
merged 2 commits into from
Jan 13, 2021
Merged

fix: update aegir and fix types #79

merged 2 commits into from
Jan 13, 2021

Conversation

hugomrdias
Copy link
Member

No description provided.

src/index.js Outdated
@@ -113,9 +113,13 @@ function validEncode (name, buf) {
* @throws {Error} Will throw if the encoding is not supported
*/
function encoding (nameOrCode) {
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth to add the reason for ignoring next to the @ts-ignore

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better & clearer to cast types (instead of adding these ignores) as following example illustrates

/**
 * @param {BaseNameOrCode} nameOrCode
 * @returns {Base}
 */
function encoding (nameOrCode) {
  const id = /** @type {BaseName & BaseCode} */(nameOrCode)
  if (constants.names[id]) {
    return constants.names[id]
  } else if (constants.codes[id]) {
    return constants.codes[id]
  } else {
    throw new Error(`Unsupported encoding: ${nameOrCode}`)
  }
}

@@ -21,7 +21,7 @@
"main": "src/index.js",
"repository": "github:multiformats/js-multibase",
"scripts": {
"prepare": "aegir build",
"prepare": "aegir build --no-bundle",
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to keep the prepare script around? Or only when in development for testing purposes?

Copy link
Contributor

Choose a reason for hiding this comment

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

It believe it is in place to use git url in dependent packages.

Copy link
Member Author

Choose a reason for hiding this comment

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

doesnt hurt i guess, dunno maybe we should discuss it

@@ -21,7 +21,7 @@
"main": "src/index.js",
"repository": "github:multiformats/js-multibase",
"scripts": {
"prepare": "aegir build",
"prepare": "aegir build --no-bundle",
Copy link
Contributor

Choose a reason for hiding this comment

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

It believe it is in place to use git url in dependent packages.

src/index.js Outdated
@@ -113,9 +113,13 @@ function validEncode (name, buf) {
* @throws {Error} Will throw if the encoding is not supported
*/
function encoding (nameOrCode) {
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better & clearer to cast types (instead of adding these ignores) as following example illustrates

/**
 * @param {BaseNameOrCode} nameOrCode
 * @returns {Base}
 */
function encoding (nameOrCode) {
  const id = /** @type {BaseName & BaseCode} */(nameOrCode)
  if (constants.names[id]) {
    return constants.names[id]
  } else if (constants.codes[id]) {
    return constants.codes[id]
  } else {
    throw new Error(`Unsupported encoding: ${nameOrCode}`)
  }
}

src/types.ts Outdated Show resolved Hide resolved
Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

LGTM

@hugomrdias hugomrdias merged commit 93ab644 into master Jan 13, 2021
@hugomrdias hugomrdias deleted the fix/update-types branch January 13, 2021 15:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants