-
Notifications
You must be signed in to change notification settings - Fork 21
feat(sdk-docs): add documentation configuration for SDK monorepo #522
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
base: main
Are you sure you want to change the base?
Conversation
@jeffersonBastos is attempting to deploy a commit to the cow Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis update restructures documentation and build configurations. It adds new documentation category files, updates markdown note syntax, adjusts schema import paths, and broadens the Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant Docusaurus
participant TypeDoc
participant SDK Packages
Developer->>Docusaurus: Run build script
Docusaurus->>TypeDoc: Generate docs for each SDK package (per config)
TypeDoc->>SDK Packages: Parse TypeScript entry points
TypeDoc-->>Docusaurus: Output documentation to specified directories
Docusaurus-->>Developer: Serve or build complete documentation site
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (3)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (2)
docs/cow-amm/tutorials/cow-amm-for-solvers.md (1)
80-84
: Avoid hard-pinned commit links
Linking to a specific commit hash (95ecc4e…
) in the services repo will go stale as soon as that file moves. Prefer a permalink to the file on the default branch (e.g.main
) or to a tagged release so the docs stay valid after future refactors.- Solvers using the non co-located driver should add a [`JitTrade`](https://github.com/cowprotocol/services/blob/95ecc4e01b7fd06ec0b71c6486cb2cdd962e5040/crates/solvers/openapi.yml#L744C1-L774C52) ... + Solvers using the non co-located driver should add a [`JitTrade`](https://github.com/cowprotocol/services/blob/main/crates/solvers/openapi.yml#L744C1-L774C52) ...package.json (1)
11-11
: Consider making memory allocation configurableThe hardcoded memory allocation might not be suitable for all environments. Consider making it configurable through an environment variable.
- "build": "yarn build:external && NODE_OPTIONS=\"--max-old-space-size=8192\" docusaurus build", + "build": "yarn build:external && NODE_OPTIONS=\"--max-old-space-size=${DOCUSAURUS_NODE_MEMORY:-8192}\" docusaurus build",This allows users to override the memory allocation if needed:
DOCUSAURUS_NODE_MEMORY=4096 yarn build
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/cow-amm/tutorials/cow-amm-for-solvers.md
(1 hunks)docs/cow-protocol/reference/core/intents/app_data.mdx
(1 hunks)docs/cow-protocol/reference/sdks/.gitignore
(1 hunks)docs/cow-protocol/reference/sdks/core-utilities/_category_.json
(1 hunks)docs/cow-protocol/reference/sdks/protocol-components/_category_.json
(1 hunks)docs/cow-protocol/reference/sdks/provider-adapters/_category_.json
(1 hunks)docs/governance/fees/fees.md
(1 hunks)docusaurus.config.ts
(2 hunks)package.json
(2 hunks)
🔇 Additional comments (5)
docs/governance/fees/fees.md (1)
7-12
: Consistent admonition syntax – well done
Switching to the:::note
/:::
triple-colon form fixes Docusaurus rendering issues and aligns with the rest of the docs.Also applies to: 48-55
docs/cow-protocol/reference/core/intents/app_data.mdx (1)
7-8
: Absolute path may break the MDX build
The import now starts with/external/…
, which webpack treats as an absolute file-system path. UnlesswebpackAlias["@site"]
(or similar) remaps that folder, the build will fail on non-Unix hosts and in CI containers.Consider importing relative to the repo root with the
@site
alias that Docusaurus exposes:-import appDataSchema from '/external/cow-sdk/packages/app-data/schemas/v1.3.0.json'; +import appDataSchema from '@site/external/cow-sdk/packages/app-data/schemas/v1.3.0.json';Please run
pnpm build:docs
(or the project’s equivalent) to confirm the change compiles.docs/cow-protocol/reference/sdks/provider-adapters/_category_.json (1)
1-8
: Category file looks good
Label, position, and generated-index settings match the new docs structure. No issues spotted.docs/cow-protocol/reference/sdks/core-utilities/_category_.json (1)
1-8
: Category definition approved
Consistent with other_category_.json
files and correctly ordered.docs/cow-protocol/reference/sdks/protocol-components/_category_.json (1)
1-9
: LGTM!The category configuration is properly structured and aligns with the documentation reorganization objectives.
entryPoints: ['./external/cow-sdk/packages/providers/ether-v5-adapter/src/index.ts'], | ||
tsconfig: './external/cow-sdk/packages/providers/ether-v5-adapter/tsconfig.json', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix naming inconsistency for ethers adapters
The source paths use ether-v5-adapter
and ether-v6-adapter
(without 's'), while the output paths and sidebar labels use ethers
(with 's'). This inconsistency could cause confusion.
Either rename the source directories to include the 's' or update the output paths and labels to match the source naming:
- out: 'cow-protocol/reference/sdks/provider-adapters/sdk-ethers-v5-adapter',
+ out: 'cow-protocol/reference/sdks/provider-adapters/sdk-ether-v5-adapter',
sidebar: {
- categoryLabel: 'sdk-ethers-v5-adapter',
+ categoryLabel: 'sdk-ether-v5-adapter',
Also applies to: 291-291, 293-293, 304-305, 308-308, 310-310
🤖 Prompt for AI Agents
In docusaurus.config.ts around lines 287-288 and also lines 291, 293, 304-305,
308, and 310, there is a naming inconsistency between 'ether-v5-adapter'
(without 's') in source paths and 'ethers' (with 's') in output paths and
sidebar labels. To fix this, either rename all source directories to include the
's' consistently as 'ethers-v5-adapter' and 'ethers-v6-adapter', or update all
output paths and sidebar labels to use 'ether' without the 's' to match the
source directories. Ensure all references are consistent across these lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeffersonBastos shouldn't it be ethers
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alfetopito Yes, it should all be "ethers" actually.
The one without the "s" is the folder name in the SDK. Would you like me to make that change?
"build:external:appdata": "yarn git:clone:appdata && cd external/app-data && yarn install --frozen --ignore-scripts && yarn compile", | ||
"build:external": "yarn build:external:cowsdk && yarn build:external:appdata", | ||
"build": "yarn build:external && docusaurus build", | ||
"build:external:cowsdk": "cd external/cow-sdk && pnpm install && pnpm run build", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Ensure pnpm is available in the build environment
The build script has switched from yarn
to pnpm
. Ensure that pnpm is installed and available in all environments where the documentation is built (CI/CD, local development).
🏁 Script executed:
#!/bin/bash
# Check if pnpm is available
which pnpm || echo "pnpm not found"
# Check if there's any CI configuration that might need updating
fd -e yml -e yaml . .github/workflows/ | xargs grep -l "yarn" | head -5
Length of output: 219
Add pnpm installation to CI and local setup
Our check showed that pnpm
isn’t available by default (sandbox returned “pnpm not found”) and no CI workflows still reference yarn
. To avoid broken builds, ensure every environment installs pnpm
before running the build:external:cowsdk
script:
• CI/CD workflows (e.g. in .github/workflows/*.yml):
– Add a step to install or set up pnpm, for example:
yaml - name: Setup Node.js uses: actions/setup-node@v3 with: node-version: '18' - name: Setup pnpm uses: pnpm/action-setup@v2 with: version: 'latest'
• Local development / docs:
– Update README or CONTRIBUTING to note that pnpm is required (e.g. npm install -g pnpm
).
These changes will guarantee pnpm
is present wherever the build script runs.
🤖 Prompt for AI Agents
In package.json at line 9, the build script uses pnpm but pnpm is not guaranteed
to be installed in CI or local environments. To fix this, update all CI/CD
workflow YAML files (e.g., in .github/workflows/) to include steps that install
or set up pnpm before running build scripts, using actions like
pnpm/action-setup@v2. Also, update the README or CONTRIBUTING documentation to
instruct developers to install pnpm locally, for example by running npm install
-g pnpm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
@jeffersonBastos if pnpm is now a requirement for the build, let's add it to the dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
- I've added the buildCommand: "npm install -g pnpm && yarn build" to vercel.json to ensure pnpm is available during Vercel deployments, and updated the documentation to instruct developers to install pnpm locally.
However, I noticed that this project doesn't have any GitHub Actions CI workflows that run builds - the existing workflows only handle CLA, project board management, and Slack notifications. The project appears to rely entirely on Vercel for deployment.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
- Add pnpm installation to Vercel build command - Update documentation to include pnpm requirement
Description
This PR refactors the SDK section in the documentation to reflect the new monorepo structure.
It also introduces three new tabs to organize the packages as follows:
├── cow-sdk (umbrella)
│
|── Provider Adapters
│ ├── cow-ethers-v5-adapter
│ ├── cow-ethers-v6-adapter
│ └── cow-viem-adapter
│
|── Core Utilities
│ ├── cow-common
│ ├── cow-config
│ └── cow-contracts-ts
│
|── Protocol Components
│ ├── sdk-order-signing
│ ├── sdk-order-book
│ ├── sdk-app-data
│ ├── sdk-trading
│ ├── sdk-composable
│ ├── sdk-shed
│ └── sdk-bridging
│
└── app-data
Note:
This PR is still referencing the main branch. Because of that, the full SDK feature branch must be merged into main before this PR can be finalized.
https://github.com/cowprotocol/cow-sdk/tree/SDK-AGNOSTIC-LIB
Changes
The documentation structure was refactored to include three separate sections for individual package documentation. This change was required because TypeDoc's type resolution system cannot properly handle cross-package references when documenting only the umbrella package with aggregated exports.
By documenting each package independently, we ensure:
Related Issues
cowprotocol/cow-sdk#403
Summary by CodeRabbit
Summary by CodeRabbit
Documentation
Chores