Use BN type instead of value

[talentlessguy]

At the moment, this module fails to import in Deno because of a type error:

error: TS2749 [ERROR]: 'BN' refers to a value, but is being used as a type here. Did you mean 'typeof BN'?
export declare type Input = Buffer | string | number | bigint | Uint8Array | BN | List | null;

this PR adds a typeof keyword, so this works on both Node.js and Deno

[ryanio]

hey, thanks for opening this issue. I believe since we use @types/bn.js for the types in Node.js it works without needing the typeof BN, and this is what we use across our other packages.

i wonder if we would need to move these types to dependencies from devDeps for Deno to be able to pick them up, or I wonder why it works fine for our other consumers in typescript/node but fails for you there.

I haven't used Deno yet but would like to learn or investigate more. could you try adding @types/bn.js to Deno to see if that helps resolve the type issue? (if that is possible, again I'm not too familiar with deno yet)

[talentlessguy]

@ryanio importing bn.js works flawlessly

import BN from 'https://esm.sh/bn.js'

console.log(BN)

I'm not sure if I was clear but the reason why this error appears is not because of @types/bn.js but because you import it with require, so it imports an actual value. For type imports you should do either this:

import type { BN } from 'bn.js'

or this:

import BN = require('bn.js')

type BNType = typeof BN

btw why the source mixed imports and requires? TypeScript will transpile imports to CJS automatically so you can safely use ES6 imports:

import BN from 'bn.js'

[ryanio]

@ryanio importing bn.js works flawlessly

import BN from 'https://esm.sh/bn.js'

console.log(BN)

is there any type information available though? as far as I know bn.js doesn't have any type definition files, which is why we use @types/bn.js

I'm not sure if I was clear but the reason why this error appears is not because of @types/bn.js but because you import it with require, so it imports an actual value. For type imports you should do either this:

import type { BN } from 'bn.js'

or this:

import BN = require('bn.js')

type BNType = typeof BN

btw why the source mixed imports and requires? TypeScript will transpile imports to CJS automatically so you can safely use ES6 imports:

import BN from 'bn.js'

I believe we use require when there is no type information available for the module, otherwise TypeScript complains. We use import as much as possible, but fall back to require in cases of javascript-only modules with no .d.ts files.

I wonder if updating to import BN from 'bn.js' would fix this issue though, that seems to be what we do in our util package which we re-export.

[talentlessguy]

@ryanio sorry I forgot to tell, esm.sh is a CDN for Deno and it automatically maps @types packages to regular ones in case they are js and have type defs, so yes, it works this way

regarding "TypeScript complains", I think there's a setting to disable erroring on modules without typings, see "skipLibCheck"

[talentlessguy]

is there anything preventing this from being merged?

[talentlessguy]

I updated it to be the same as in the util package, please take a look @ryanio

[ryanio]

@talentlessguy thanks! and this change made it start working in Deno for you?

[talentlessguy]

@ryanio looks like tsconfig is wrongly configured?

[talentlessguy]

also I cannot test this in Deno since it's not published as an npm package

[ryanio]

ah yes I just checked and the @ethereumjs/config packages are outdated, it should be on v2 which has the esMouduleInterop flag, but we are no longer maintaining them since we have a base ./config folder in our monorepo now.

Can you add the esModuleInterop flag to the tsconfig in this PR?

We have considered bringing this lib into the monorepo but it doesn't receive much updates so that's why we haven't for now.

[talentlessguy]

@ryanio done

[ryanio]

@ryanio done

@talentlessguy esModuleInterop didn't work? I see you added allowSyntheticDefaultImports instead

[coveralls]

Coverage remained the same at 90.598% when pulling 44193fe on talentlessguy:patch-1 into 53a2165 on ethereumjs:master.

[talentlessguy]

@ryanio esModuleInterop includes synthetic default imports

and it builds fine

[ryanio]

looks like it's getting stuck on karma now, can you try adding karmaTypescriptConfig: { tsconfig: './tsconfig.json' } to karma.conf.js

regarding trying it in deno, maybe https://gitpkg.vercel.app/ could help?

[talentlessguy]

@ryanio Karma seems to be broken completely

and not sure if it's my fault, looks like it was broken before me

> [email protected] test:browser /home/v1rtl/Coding/forks/rlp
> karma start karma.conf.js

(node:47585) Warning: Accessing non-existent property 'VERSION' of module exports inside circular dependency
(Use `node --trace-warnings ...` to show where the warning was created)
26 09 2021 00:08:32.710:INFO [compiler.karma-typescript]: Compiling project using Typescript 3.9.10
26 09 2021 00:08:35.060:INFO [compiler.karma-typescript]: Compiled 5 files in 2320 ms.
26 09 2021 00:08:35.818:ERROR [karma-server]: UncaughtException:: Unable to resolve module [process] from [/home/v1rtl/Coding/forks/rlp/test/dataTypes.spec.js]
{
  "basedir": "/home/v1rtl/Coding/forks/rlp",
  "extensions": [

[talentlessguy]

@ryanio gitpkg is for npm, not for Deno and Deno doesn't support importing from github files either if they have relative dependencies (such as bn.js here).

[talentlessguy]

@ryanio is there anything else needs to be fixed for this to get merged?

[talentlessguy]

@ryanio ping

plzzzz merge

it's a big blocker for deno :((

[ryanio]

@talentlessguy sorry for the delay, we are focusing on the merge work in our monorepo this upcoming week but we will try to get to it after - do you need a npm release or is just merging to master is fine for your needs? because I could do that more immediately. (cc @holgerd77)

can you also update from require to import BN in src/index.ts and test/official.spec.ts?

thanks!

[talentlessguy]

@ryanio ok I will, one sec

[talentlessguy]

@ryanio done

[ryanio]

@talentlessguy thanks! could you clarify if you need this released on npm or would merge to master be suitable for now

[talentlessguy]

@ryanio it has to be released on npm beause esm.sh transforms npm packages

[talentlessguy]

@ryanio ping

please release the package ......

[ryanio]

our team is juggling a lot right now we will try to get to it soon! also the ci is failing on lint, could you run lint:fix?

[talentlessguy]

@ryanio done

[ryanio]

🙏

[ryanio]

lgtm, thanks!

[talentlessguy]

@ryanio omg thanks for merging! could you release a patch version now

[ryanio]

yes will do now :)

[ryanio]

@talentlessguy have published v2.2.7 on npm, thanks for your continued patience and follow up :)

[talentlessguy]

@ryanio omg omg thanks

one more thing

could you please bump rlp version on ethereumjs-util?

[ryanio]

@talentlessguy since this was a patch release and it is specified in ethereumjs-util with a carat it should automatically pick it up on new install