feat!: export object instead of default export #16

vmx · 2021-03-29T15:19:18Z

Default exports don't work well with CommonJS and TypeScript. Hence replace
the default export with an object export.

BREAKING CHANGE: import of dag-cbor changed

Prior to this change this module was imported as

import dagcbor from '@ipld/dag-cbor'

Now it is imported as

import * as dagcbor from `@ipld/dag-cbor'

Default exports don't work well with CommonJS and TypeScript. Hence replace the default export with an object export. BREAKING CHANGE: import of dag-cbor changed Prior to this change this module was imported as ```js import dagcbor from '@ipld/dag-cbor' ``` Now it is imported as ```js import * as dagcbor from `@ipld/dag-cbor' ```

vmx · 2021-03-29T15:19:56Z

This is a change along the lines of multiformats/js-multiformats#70.

Gozala

Looks good, but could you please make sure that name and code will get dag-cbor and 0x71 as types as opposed to string and number. Comment in the diff suggests multiple ways to do it.

Gozala · 2021-03-29T18:50:18Z

index.js

@@ -104,4 +104,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)


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.

annotating those at definition site is also another option.

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.

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)

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 ...

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 })

Gozala · 2021-03-30T23:38:04Z

index.js

@@ -104,4 +104,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)


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 })

Gozala · 2021-03-30T23:39:29Z

index.js


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


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.

now that's a weird annotation!

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

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

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.

Gozala · 2021-03-30T23:39:47Z

index.js

 const code = 0x71
+/** @type {string} */


Suggested change

/** @type {string} */

/** @type {'dag-cbor'} */

rvagg · 2021-04-07T12:26:37Z

building further on this in #18

rvagg · 2021-04-26T04:30:56Z

squashed this into #18 and published

vmx requested review from Gozala and rvagg March 29, 2021 15:35

Gozala requested changes Mar 29, 2021

View reviewed changes

alanshaw mentioned this pull request Mar 30, 2021

fix: example on README #17

Closed

chore: use more generic types

a104461

Gozala requested changes Mar 30, 2021

View reviewed changes

rvagg mentioned this pull request Apr 7, 2021

feat!: full type checks, generation and publishing #18

Merged

rvagg mentioned this pull request Apr 10, 2021

[CoreTypes] Tracking protocol/w3dt-sudo#17

Open

77 tasks

rvagg closed this Apr 26, 2021

rvagg deleted the export-object branch April 26, 2021 04:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: export object instead of default export #16

feat!: export object instead of default export #16

vmx commented Mar 29, 2021

vmx commented Mar 29, 2021

Gozala left a comment

Gozala Mar 29, 2021

Gozala Mar 29, 2021

vmx Mar 30, 2021

vmx Mar 30, 2021

Gozala Mar 30, 2021

Gozala Mar 30, 2021

Gozala Mar 30, 2021

Gozala Mar 30, 2021

rvagg Mar 31, 2021

vmx Apr 1, 2021

Gozala Apr 2, 2021

vmx Apr 6, 2021

Gozala Mar 30, 2021

rvagg commented Apr 7, 2021

rvagg commented Apr 26, 2021

feat!: export object instead of default export #16

feat!: export object instead of default export #16

Conversation

vmx commented Mar 29, 2021

vmx commented Mar 29, 2021

Gozala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvagg commented Apr 7, 2021

rvagg commented Apr 26, 2021