ceramicnetwork / js-dag-jose

Other
46 stars 9 forks source link

Use Signer type from did-jwt #3

Closed PaulLeCam closed 4 years ago

PaulLeCam commented 4 years ago

The main change is to import the Signer type from the did-jwt package: https://github.com/ceramicnetwork/js-dag-jose/compare/update-types?expand=1#diff-e8b22f0b84625cc38502a381d487385dR2 This way the types can match in IDW with the signer created by the keyring.

The other changes are just formatting with prettier running on save in my editor, I can revert them if you prefer? One thing that could be useful is to create our own ESLint configs we could use by default across projects, so we can have more consistency. I created this package for Mainframe projects that has been pretty useful and easy to maintain, mostly bumping major dependencies every once in a while: https://github.com/MainframeOS/eslint-config-mainframe

oed commented 4 years ago

Makes sense to import the type 👍

And sounds great to have an eslint config that can be used by all packages! Two things to note so that it doesn't have to change a lot of lines in our existing code:

If you feel strongly about these it's fine, this is just what we've been generally doing so it might be easier to just use that.

PaulLeCam commented 4 years ago

We've been using functions with a space before parenthesis:

function test (param: string) {}

As far as I know it's not an available option with Prettier, so if we want to keep it this way we can't use Prettier.

We have not been using , on the last property of objects

👍 OK it will be easy to change as it's an option in Prettier

If you feel strongly about these it's fine, this is just what we've been generally doing so it might be easier to just use that.

I don't feel strongly about it, I mostly like the convenience of using Prettier so the editor can take care of formatting rather than me. I usually make the following changes from the default options though:

I don't mind changing any of this though, it's really about the convenience of not having to care about it in every repo once it's setup. Maybe we can create a new repo for this shared ESLint config and discuss the options in PRs on it?

oed commented 4 years ago

Thanks for clarifying! Please go ahead :)

Setting trailing comas to "all", that's just a small thing for git diffs so that only the lines that have a meaningful change are displayed, rather than also the ones with a coma added or removed, but I don't mind keeping the default

That's a good point. Might be worth changing in the long run then.

PaulLeCam commented 4 years ago

@oed I created https://github.com/3box/eslint-config-3box and updated the ESLint config to use it.

I had to move the __tests__ folder out of the src one because it was messing up the ESLint/TypeScript parser with the exclude rules I think. It should be safer anyways, because the exclusion rules didn't cover the __fixtures__ folder and they ended up being in the lib folder, making the npm package heavier than necessary.

oed commented 4 years ago

Nice! I guess we should rename __tests__ to just test if in root, and keep that consistent in all of our packages, as that seems like what more projects are doing.

PaulLeCam commented 4 years ago

Last change (I hope!): I added a signing.createString() function to be used by the DID provider in IDW to avoid transferring buffers over JSON-RPC, instead it will be decoded by js-did.

oed commented 4 years ago

Refactored this library a bunch to conform to the new dag-jose spec. Your changes should be addressed in master now.