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.

Sign up for GitHub

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

feat!: export object instead of default export #16

Closed
wants to merge 2 commits into from
Closed

feat!: export object instead of default export #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3202b5b
feat!: export object instead of default export
vmx Mar 29, 2021
a104461
chore: use more generic types
vmx Mar 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import * as cborg from 'cborg'
import CID from 'multiformats/cid'
import { codec } from 'multiformats/codecs/codec'
import { Decoder, Encoder } from 'multiformats/codecs/codec'

// https://github.com/ipfs/go-ipfs/issues/3570#issuecomment-273931692
const CID_CBOR_TAG = 42
/** @type {number} */
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to use actual literal type here, so TS won't generalize it to number.

Suggested change
/** @type {number} */
/** @type {0x71} */

Note: This is not needed if codec({ code: 0x71, ... is used as TS can infer that.

Copy link
Member

Choose a reason for hiding this comment

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

now that's a weird annotation!

Copy link
Member Author

Choose a reason for hiding this comment

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

Without the annotation it also isn't generalaized. @Gozala I misunderstood you, I thought you want that generalization.

Copy link
Contributor

Choose a reason for hiding this comment

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

I’m pretty sure TS generalizes type of code in export const code = 0x71 to number.

it did not generalized it in original code codec({ code: 0x71, ... }) because codec user generics.

I apologize for confusion, but I do want non generalized types as it helps with compositions allowing codecs to reject incompatible blocks at compile time

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. The code (without the generalization) produces:

export const name: "dag-cbor";
export const code: 113;

which I thought is a sign that it would also use those specific types directly and not the generalized ones. The reason why I though it is that the generalized version produces:

/** @type {string} */
export const name: string;
/** @type {number} */
export const code: number;

which is clearly generalized.

@Gozala I just checked it with your proposed type annotations, it produces:

/** @type {'dag-cbor'} */
export const name: 'dag-cbor';
/** @type {0x71} */
export const code: 0x71;

Isn't that the same as the non-generalized version?

Anyway, please tell me which version, the one without any type annotion or the one with your type annotation, would be preferred and I'll push an updated version.

const code = 0x71
/** @type {string} */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** @type {string} */
/** @type {'dag-cbor'} */

const name = 'dag-cbor'

// this will receive all Objects, we need to filter out anything that's not
Expand Down Expand Up @@ -104,4 +106,7 @@ function decode (data) {
return cborg.decode(data, decodeOptions)
}

export default codec({ name, code, encode, decode })
const decoder = new Decoder(name, code, decode)
const encoder = new Encoder(name, code, encode)
Copy link
Contributor

Choose a reason for hiding this comment

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

I’d suggest following instead:

export const {name, code, encode, decode, encoder, decoder} = codec({ name, code, encode, decode })

That is because this would result in code and name getting types for corresponding literals as opposed to generalized number and string they get here.

Copy link
Contributor

Choose a reason for hiding this comment

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

annotating those at definition site is also another option.

Copy link
Member Author

Choose a reason for hiding this comment

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

export const {name, code, encode, decode, encoder, decoder} = codec({ name, code, encode, decode })

I tried that. But it fails to build (SyntaxError: Identifier 'name' has already been declared (112:14)). I thought it's a JS error, but now I realize it's probably a problem of our toolchain. I get:

$ npm run build

> @ipld/dag-cbor@0.0.0-dev build
> npm run build:js && npm run build:types


> @ipld/dag-cbor@0.0.0-dev build:js
> npm_config_yes=true npx ipjs@latest build --tests

parsing file:///home/vmx/src/pl/js-dag-cbor/index.js
file:///home/vmx/.npm/_npx/ef1ca190cb7ddd1d/node_modules/acorn/dist/acorn.mjs:3010
  var err = new SyntaxError(message);
            ^

SyntaxError: Identifier 'name' has already been declared (112:14)
    at Parser.pp$4.raise (file:///home/vmx/.npm/_npx/ef1ca190cb7ddd1d/node_modules/acorn/dist/acorn.mjs:3010:13)
    at Parser.pp$5.declareName (file:///home/vmx/.npm/_npx/ef1ca190cb7ddd1d/node_modules/acorn/dist/acorn.mjs:3084:26)
    at Parser.pp$2.checkLValSimple (file:///home/vmx/.npm/_npx/ef1ca190cb7ddd1d/node_modules/acorn/dist/acorn.mjs:1899:48)
    at Parser.pp$2.checkLValPattern (file:///home/vmx/.npm/_npx/ef1ca190cb7ddd1d/node_modules/acorn/dist/acorn.mjs:1941:10)
    at Parser.pp$2.checkLValInnerPattern (file:///home/vmx/.npm/_npx/ef1ca190cb7ddd1d/node_modules/acorn/dist/acorn.mjs:1963:10)
    at Parser.pp$2.checkLValInnerPattern (file:///home/vmx/.npm/_npx/ef1ca190cb7ddd1d/node_modules/acorn/dist/acorn.mjs:1951:10)
    at Parser.pp$2.checkLValPattern (file:///home/vmx/.npm/_npx/ef1ca190cb7ddd1d/node_modules/acorn/dist/acorn.mjs:1928:10)
    at Parser.pp$1.parseVarId (file:///home/vmx/.npm/_npx/ef1ca190cb7ddd1d/node_modules/acorn/dist/acorn.mjs:1256:8)
    at Parser.pp$1.parseVar (file:///home/vmx/.npm/_npx/ef1ca190cb7ddd1d/node_modules/acorn/dist/acorn.mjs:1238:10)
    at Parser.pp$1.parseVarStatement (file:///home/vmx/.npm/_npx/ef1ca190cb7ddd1d/node_modules/acorn/dist/acorn.mjs:1104:8) {
  pos: 2740,
  loc: Position { line: 112, column: 14 },
  raisedAt: 2787
}

So I guess annotating them is the quicker way.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is a JS thing, I tried with Node.js:

$ node index.js
file:///home/vmx/src/pl/js-dag-cbor/index.js:112
export const {name, code, encode, decode, encoder, decoder} = codec({ name, code, encode, decode })
              ^

SyntaxError: Identifier 'name' has already been declared
    at Loader.moduleStrategy (node:internal/modules/esm/translators:147:18)
    at async link (node:internal/modules/esm/module_job:48:21)

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried that. But it fails to build (SyntaxError: Identifier 'name' has already been declared (112:14)). I thought it's a JS error, but now I realize it's probably a problem of our toolchain.

Ah that makes sense because we define const name and const code so it rejects const { name, code ...

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 @rvagg made a change to a codec function so that it returns encoder and decoder fields, with that following should work:

export const { name, code, decoder, encoder } = codec({ name: 'dag-cbor', code: 0x71, encode, decode })


export { name, code, decode, encode, decoder, encoder }
2 changes: 1 addition & 1 deletion test/test-basics.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
'use strict'
import garbage from 'ipld-garbage'
import chai from 'chai'
import dagcbor from '../index.js'
import * as dagcbor from '../index.js'
import { bytes, CID } from 'multiformats'

const { encode, decode } = dagcbor
Expand Down