ecadlabs / taquito

A library for building dApps on the Tezos Blockchain - JavaScript / TypeScript
https://taquito.io
Apache License 2.0
298 stars 116 forks source link

Dependency missing from package.json #2391

Open sirrocco opened 1 year ago

sirrocco commented 1 year ago

Description In the file: https://github.com/ecadlabs/taquito/blob/18ab9d890f32370bc0eaf6bb9fa491577689de0e/packages/taquito-signer/src/derivation-tools/ecdsa.ts#L6

there's a dependency on bn.js however the package is not specified in package.json - this causes an exception when using yarn pnp.

Steps To Reproduce Steps to reproduce the behavior:

  1. Create a project using yarn pnp
  2. Try to run a script that uses a function from the signer package
  3. See the error:
Error: @taquito/signer tried to access bn.js, but it isn't declared in its dependencies; this makes the require call ambiguous and unsound.

Same situation for: @taquito/local-forging which tries to access @taquito/rpc in validator.ts and interface.ts

dsawali commented 1 year ago

Hello @sirrocco,

The dependency you mentioned is not missing, it's present here: https://github.com/ecadlabs/taquito/blob/master/packages/taquito-signer/package.json#L78

Also please note that we don't officially support yarn builds, we recommend using Taquito with NPM.

Could you also post your development environment (OS, Node version, NPM version, etc)?

sirrocco commented 1 year ago

Hi @dsawali,

That's the type definition package not the actual bn.js package that's added as a dependency.

Happens on any OS - NPM v9.5, Node v18.15.

Thanks.

dsawali commented 1 year ago

@sirrocco let me see if I can reproduce that on my end. What Taquito version were you running?

sirrocco commented 1 year ago

V15.1 - I'll try to duplicate it in a clean repo and ping you when I have it. (Probably tomorrow)

Thanks

sirrocco commented 1 year ago

Hi @dsawali - here you go: https://github.com/sirrocco/taquito-yarn-pnp-issue

You should be able to just run yarn start and see the error. You can then change .yarnrc.yml and uncomment pnpMode: loose and run yarn install - it will still log the error but the script will finish

Innkst commented 1 year ago

@taquito/local-forging which tries to access @taquito/rpc in validator.ts and interface.ts doesn't seem to have this issue.

We'll add @types/bn.js instead of bn.js and hopefully, it will resolve this issue.

dsawali commented 1 year ago

I've looked a little bit into the @types/bn.js. We use that only for type definitions and to create an object of type BN.

The usage of the package @types/bn.js is still correct.

sirrocco commented 1 year ago

What do you mean?

I'm not arguing to remove @types/bn.js but to add bn.js to package.json.

You're importing: import BN from 'bn.js'; but it's not present in the package.json.

@types/bn.js has only types so you wouldn't be able to instantiate a new BN object if the bn.js package wouldn't be available through the build system somehow.

dsawali commented 1 year ago

Sorry, I wasn't very clear.

This issue is kinda strange. I tried adding bn.js into our package.json, but the build failed because it conflicted with the type definition package.

Currently the import path for import BN from 'bn.js'; defaults to this: /node_modules/@types/bn.js/index.

Hence my confusion.

We're going to investigate this a bit more, it seems related to the difference in our module resolution. I didn't have direct involvement in this file during development, so I'm trying to gather info.