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

Configuration validation #1573

Open
achingbrain opened this issue Feb 1, 2023 · 3 comments · Fixed by #1778 · May be fixed by #2133
Open

Configuration validation #1573

achingbrain opened this issue Feb 1, 2023 · 3 comments · Fixed by #1778 · May be fixed by #2133
Labels
good first issue Good issue for new contributors help wanted Seeking public contribution on this issue kind/enhancement A net-new feature or improvement to an existing feature

Comments

@achingbrain
Copy link
Member

achingbrain commented Feb 1, 2023

We don't perform much in the way of validation on loaded libp2p config but we really should.

Ideally each module parsing it's config would use something like superstruct to validate at creation time and fail fast with good feedback to the user as to how they can fix their broken config.

This should negate the need for PRs like #1572 as the config would have been validated in src/config.ts already.

Update

@achingbrain explored a few different options and we determined that zod is perhaps the best choice. Currently Zod is 8kb minified + zipped but based on this discussion there are a few different approaches to making Zod tree-shakeable.

Anyone who takes up this work can continue with the commits on #2133 and have look at the discussion on that PR

@achingbrain achingbrain added need/triage Needs initial labeling and prioritization kind/enhancement A net-new feature or improvement to an existing feature need/analysis Needs further analysis before proceeding exp/intermediate Prior experience is likely helpful and removed need/triage Needs initial labeling and prioritization labels Feb 1, 2023
@maschad maschad self-assigned this Feb 6, 2023
@p-shahi p-shahi added this to js-libp2p Feb 7, 2023
@p-shahi p-shahi moved this to 🥞Weekly Candidates/Discuss🎙 in js-libp2p Feb 7, 2023
@maschad maschad moved this from 🥞Weekly Candidates/Discuss🎙 to 🏃‍♀️In Progress in js-libp2p Feb 8, 2023
@p-shahi p-shahi removed the need/analysis Needs further analysis before proceeding label Feb 14, 2023
@p-shahi p-shahi moved this from 🏃‍♀️In Progress to 🥞Weekly Candidates/Discuss🎙 in js-libp2p Feb 28, 2023
@maschad maschad moved this from 🥞Weekly Candidates/Discuss🎙 to 🏃‍♀️In Progress in js-libp2p Mar 27, 2023
@maschad maschad moved this from 🏃‍♀️In Progress to 🧱Blocked in js-libp2p May 1, 2023
@maschad maschad moved this from 🧱Blocked to 🏃‍♀️In Progress in js-libp2p May 1, 2023
maschad added a commit to maschad/js-libp2p that referenced this issue May 29, 2023

Verified

This commit was signed with the committer’s verified signature.
maschad Chad Nehemiah
@p-shahi p-shahi moved this from 🏃‍♀️In Progress to 🧱Blocked in js-libp2p Jun 5, 2023
@p-shahi p-shahi moved this from 🧱Blocked to todo in js-libp2p Jun 5, 2023
@maschad maschad moved this from 🛠️ Todo to 🏃‍♀️In Progress in js-libp2p Jun 26, 2023
@maschad maschad moved this from 🏃‍♀️In Progress to 🛠️ Todo in js-libp2p Jun 27, 2023
@maschad maschad moved this from 🛠️ Todo to 🏃‍♀️In Progress in js-libp2p Jun 30, 2023
@maschad maschad moved this from 🏃‍♀️In Progress to 🛠️ Todo in js-libp2p Jul 3, 2023
@maschad maschad moved this from 🛠️ Todo to 🏃‍♀️In Progress in js-libp2p Jul 3, 2023
maschad added a commit to maschad/js-libp2p that referenced this issue Jul 3, 2023

Verified

This commit was signed with the committer’s verified signature.
maschad Chad Nehemiah
maschad added a commit to maschad/js-libp2p that referenced this issue Jul 4, 2023

Verified

This commit was signed with the committer’s verified signature.
maschad Chad Nehemiah
@maschad maschad moved this from 🏃‍♀️In Progress to 🧱Blocked in js-libp2p Jul 4, 2023
@maschad maschad moved this from 🧱Blocked to 🥸In Review in js-libp2p Jul 4, 2023
maschad added a commit to maschad/js-libp2p that referenced this issue Jul 5, 2023

Verified

This commit was signed with the committer’s verified signature.
maschad Chad Nehemiah
maschad added a commit to maschad/js-libp2p that referenced this issue Jul 5, 2023

Verified

This commit was signed with the committer’s verified signature.
maschad Chad Nehemiah
maschad added a commit to maschad/js-libp2p that referenced this issue Jul 21, 2023

Verified

This commit was signed with the committer’s verified signature.
maschad Chad Nehemiah
@p-shahi p-shahi removed this from js-libp2p Aug 15, 2023
maschad added a commit to maschad/js-libp2p that referenced this issue Aug 20, 2023

Verified

This commit was signed with the committer’s verified signature.
maschad Chad Nehemiah
)
maschad added a commit to maschad/js-libp2p that referenced this issue Aug 21, 2023

Verified

This commit was signed with the committer’s verified signature.
maschad Chad Nehemiah
@achingbrain achingbrain linked a pull request Oct 6, 2023 that will close this issue
@maschad
Copy link
Member

maschad commented Jan 15, 2025

Re-opening this as original PR which addressed this was reverted and subsequent work has taken place on #2133

I've also updated the description

@maschad maschad reopened this Jan 15, 2025
@maschad maschad removed their assignment Jan 15, 2025
@maschad maschad added help wanted Seeking public contribution on this issue good first issue Good issue for new contributors and removed exp/intermediate Prior experience is likely helpful labels Jan 15, 2025
@sumanjeet0012
Copy link

@maschad I want to work on this issue.
I want to use the latest version of Zod for validation.

@achingbrain
Copy link
Member Author

It's worth reading this comment - #2133 (comment)

At the time Zod added a massive chunk of code to browser bundles, too much for us to adopt it. The situation was supposed to improve with Zod v4 but from what I can see, that still hasn't been released.

It would be good to finally close this issue out though, I would probably use superstruct instead since it's over 10x smaller than Zod.

We can switch later if there's a pressing reason to do so.

I would start with a PR that just adds validation to the libp2p module. Other modules in the monorepo can be done as follow-ups.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good issue for new contributors help wanted Seeking public contribution on this issue kind/enhancement A net-new feature or improvement to an existing feature
Projects
None yet
4 participants