Skip to content
This repository has been archived by the owner on Jan 20, 2021. It is now read-only.

Modularization #46

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Modularization #46

wants to merge 10 commits into from

Conversation

ukstv
Copy link
Contributor

@ukstv ukstv commented Dec 16, 2020

Divide the package into linking (light) and validation (heavy) subpackages.

Beware:

  • pnpm: npm and lerna suck,
  • subpackages: breaks CircleCI and automated code coverage reports

@oed
Copy link
Member

oed commented Dec 16, 2020

I like this structure generally. It's clean and keeps the validation part as one complete package. The main problem with this is that we use it in the @ceramicnetworkdoctype-caip10-link package in js-ceramic which is also used in the @ceramicnetwork/http-client which we want to keep down the size of. However, I think it might be possible to have the http-client only depend on the doctype and not the handler, which would remove this issue altogether.

Would love some more indepth thoughts on the changes @zachferland @PaulLeCam

What do we think about pnpm?

@oed
Copy link
Member

oed commented Dec 16, 2020

Here is a PR in ceramic that could potentially mitigate the issue mentioned above: ceramicnetwork/js-ceramic#668

@ukstv
Copy link
Contributor Author

ukstv commented Dec 16, 2020

Naaa-a-ah! Found the solution. Easier, but a bit weird.

@ukstv ukstv marked this pull request as draft December 16, 2020 15:33
@ukstv
Copy link
Contributor Author

ukstv commented Dec 16, 2020

This: #47 solves the trouble. Runs all right in browser with no zondax installed.

The same trick could be done to Polkadot and EOSio in order to reduce the resulting web package.

Copy link

@PaulLeCam PaulLeCam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I'm not familiar with the specifics so my comments are just about the setup.

Maybe we should discuss the use of pnpm a bit more, I've never used it before so I don't know the pros and cons compared to alternatives.
Overall though it would be good to unify the tools we're using. npm v7 supports workspaces so maybe it's something we could try out first to avoid adding pnpm to our tools unless strictly necessary?


## Install

```
$ npm install --save 3id-blockchain-utils

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should change no?

},
"author": "oed",
"license": "(Apache-2.0 OR MIT)",
"dependencies": {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we split this package into one package for each blockchain so we can make the dependencies more manageable?

- [validate](#validation) the link.

Due to asymmetry in dependencies - linking is light weigth, while validation requires quite heavy code - and target use cases,
the package is divided into two: namely `@3id-blockchain-utils/linking` and `@3id-blockchain-utils/validation`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the namespace we want to use rather than @ceramicnetwork?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use @ceramicnetwork/blockchain-linking and @ceramicnetwork/blockchain-validation

Copy link
Member

@oed oed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks like a good start!

- [validate](#validation) the link.

Due to asymmetry in dependencies - linking is light weigth, while validation requires quite heavy code - and target use cases,
the package is divided into two: namely `@3id-blockchain-utils/linking` and `@3id-blockchain-utils/validation`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use @ceramicnetwork/blockchain-linking and @ceramicnetwork/blockchain-validation

@ukstv ukstv marked this pull request as ready for review December 21, 2020 09:09
@ukstv
Copy link
Contributor Author

ukstv commented Dec 21, 2020

@oed Here it is.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants