map bytes to Uint8array #40
Conversation
|
I ran yarn build and yarn test locally to confirm this doesn't break anything, but noticed that I had a bunch of changed files not sure why. LMK if i should reset some of them, or if you want to merge them. |
There was a problem hiding this comment.
Yes you should include files generated by yarn build. These include the examples generated with the new code as well as the test example-program idl and expected codegen.
The docs fields in tests/example-program-gen/idl.json are removed in your commit which probably means you've ran anchor build manually and that's why the generated example-program code doesn't include docs. With yarn build it should use the nightly version of anchor which should include those comments.
Run yarn build and include all files. Otherwise the CI tests will fail. Also please update CHANGELOG.md.
package.json
Outdated
| @@ -51,6 +50,8 @@ | |||
| "js-sha256": "^0.9.0", | |||
| "prettier": "^2.6.2", | |||
| "snake-case": "^3.0.4", | |||
| "ts-morph": "^14.0.0" | |||
| "ts-morph": "^14.0.0", | |||
| "rimraf": "^3.0.2", | |||
There was a problem hiding this comment.
Need to move rimraf and tsc to dependencies from devDependencies else they can't be executed during postinstall stage. It allows to add anchor-client-gen as npm dependency to a project rather then requiring people to install it globally, possibly causing versioning projects between multiple projects.
There was a problem hiding this comment.
I had no problems adding it as a dependency. The npm package containes the dist directory already built.
There was a problem hiding this comment.
yeah this only is an issue when linking a package via git branch
There was a problem hiding this comment.
Hmm now that I think about it I don't think we need this (the ability to link the package via git branch and the postinstall script). If you want to get the unreleased features there's the beta package, I can update it for you if you want to use the latest commits. And if you want to use your local package you can always yarn / npm link.
In any case I think this is out of scope for this PR. Can you leave the package.json as it was for now and let's have this discussion on another PR / issue?
There was a problem hiding this comment.
Well i needed it to continue with what i wanted to work on as I'm not the maintainer of this repo. I don't think yarn link is a workable solution, when working with two other people on the same project, as it doesn't update lock files and hence produces inconsistent git commits. Reverted the commit anyways.
There was a problem hiding this comment.
Thanks. Yea I think it's a valid use case and would be happy to accommodate for this but let's discuss it separately.
src/common.ts
Outdated
| return `${paramPrefix}${ty.name}` | ||
| case "bytes": | ||
| return `Buffer.from(${paramPrefix}${ty.name})` |
There was a problem hiding this comment.
Shouldn't this be Uint8Array.from(<>)? Buffer is not available in browsers.
There was a problem hiding this comment.
yes, it's definitely available due to some dependency / polyfill that is included, but agreed, can use the less specific type here
tests/test.ts
Outdated
| @@ -99,7 +99,7 @@ test("init and account fetch", async () => { | |||
| expect( | |||
| res.i128Field.eq(new BN("-85070591730234615865843651857942052874")) | |||
| ).toBe(true) | |||
| expect(res.bytesField).toStrictEqual([1, 2, 255, 254]) | |||
| expect(res.bytesField).toEqual(Buffer.from([1, 2, 255, 254])) | |||
There was a problem hiding this comment.
Could we use Uint8Array.from(...) in this filed instead of Buffer? I know Buffer is a subclass of Uint8Array but it's a bit confusing.
There was a problem hiding this comment.
The bytes field is actually a buffer, and the comparison fails if the right hand side is not a buffer either.
There was a problem hiding this comment.
Hmm, does the borsh layout decode return a Buffer instead of Uint8Array in that case?
There was a problem hiding this comment.
Yeah borsh seems to be pretty much Buffer based.
|
I ran yarn build, but the npm package doesn't work on my platform (darwin/arm64): |
|
Ah that makes sense. It ran your globally installed version of anchor which is < anchor 0.25 and that's why the docs aren't generated. |
This reverts commit 1518bd7.
|
Alright, looks good overall! I'll finish it up from here with the build. Thanks for the PR! |
Uint8Array is a js native representation for an array of unsigned bytes, the current Array mapping has bad performance characteristics, especially when working with large buffers. I'd rather leave the Array conversion to the user than making it part of the generator.