Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: introduces new package @dfinity/core #681

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

krpeacock
Copy link
Contributor

Description

Since it was becoming a pain to install the individual packages, since most of them were already necessary for the auth-client anyway, this PR introduces @dfinity/core, a bundle of

  • @dfinity/agent
  • @dfinity/candid
  • @dfinity/principal
  • @dfinity/identity
  • @dfinity/utils

So you can rapidly get to work building instead of maintaining versions. The individual packages will still be maintained and released separately for projects that have specific needs and want to reduce bloat.

Checklist:

  • My changes follow the guidelines in CONTRIBUTING.md.
  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 24, 2023

size-limit report 📦

Path Size
@dfinity/agent 89.94 KB (0%)
@dfinity/candid 15.46 KB (0%)
@dfinity/principal 6.77 KB (0%)
@dfinity/auth-client 95.46 KB (0%)
@dfinity/assets 92.07 KB (0%)
@dfinity/identity 92.35 KB (0%)
@dfinity/identity-secp256k1 230.06 KB (0%)

@krpeacock krpeacock changed the title @dfinity/core feat: introduces new package @dfinity/core Jan 24, 2023
@peterpeterparker
Copy link
Member

peterpeterparker commented Jan 25, 2023

Not sure @dfinity/utils should be embedded. I like the idea that we provide it to more devs, because useful, but most do not use it currently. They don't necessecary have to, on the contrary of the other libs here that are almost mandatory to implement anything on the IC.

So yeah, I'm hesitant. If we do so we should probably move it over to https://github.com/dfinity/agent-js repo (not against) and split one of two things that are not utils per sé.

@lmuntaner
Copy link
Contributor

Not sure @dfinity/utils should be embedded. I like the idea that we provide it to more devs, because useful, but most do not use it currently. They don't necessecary have to, on the contrary of the other libs here that are almost mandatory to implement anything on the IC...

I agree with @peterpeterparker, I'm not sure that utils should be part of the core. It's very specific to NNS, except for the helpers defaultAgent, createAgent and createServices.

Not a strong opinion either, but I also agree on moving it here if we do so.

@krpeacock
Copy link
Contributor Author

My view is that there should be a base set of utils, that even candid, principal, and agent depend on. Of the items in utils, I imagine the following are all broadly useful to people:

I also would support moving the buffer and leb utility methods out of candid and into utils

@lmuntaner
Copy link
Contributor

@krpeacock @peterpeterparker I came across this again.

Should we continue with this? Did something change?

@krpeacock
Copy link
Contributor Author

@lmuntaner After doing some exploration of other packages, I decided that it would be a better approach to fold all of the packages into @dfinity/agent, and to have all the exports be from subdirectories instead of the root. Basically the same structure that https://www.npmjs.com/package/@noble/curves uses.

The refactor was more than I could pull off in a cooldown week (I did try), so I'd need to prioritize it as a feature if I wanted to get this through

@lmuntaner
Copy link
Contributor

@krpeacock that makes sense.

Does @dfinity/agent use all those utils? I'm not so sure all of them fit so well there though. For example:
assertNonNullish
toNullable
fromNullable
fromDefinedNullable

Those seem from a different domain than agent. Maybe inside @dfinity/candid?

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

Successfully merging this pull request may close these issues.

3 participants