-
Notifications
You must be signed in to change notification settings - Fork 24
fix: update aegir and fix types #79
Conversation
src/index.js
Outdated
| * @throws {Error} Will throw if the encoding is not supported | ||
| */ | ||
| function encoding (nameOrCode) { | ||
| // @ts-ignore |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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}`)
}
}| "repository": "github:multiformats/js-multibase", | ||
| "scripts": { | ||
| "prepare": "aegir build", | ||
| "prepare": "aegir build --no-bundle", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| "repository": "github:multiformats/js-multibase", | ||
| "scripts": { | ||
| "prepare": "aegir build", | ||
| "prepare": "aegir build --no-bundle", |
There was a problem hiding this comment.
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
| * @throws {Error} Will throw if the encoding is not supported | ||
| */ | ||
| function encoding (nameOrCode) { | ||
| // @ts-ignore |
There was a problem hiding this comment.
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}`)
}
}
vasco-santos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.