refactor!: remove builders and formatters re-export#11361
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
📝 WalkthroughWalkthroughRemoved root re-exports of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
|
I need a hand here, I'm not sure how to deal with this: Only usage discord.js/packages/discord.js/typings/index.d.ts Lines 4996 to 5005 in 8573d8b Do we just move that type to |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/discord.js/package.json(1 hunks)packages/discord.js/typings/index.d.ts(2 hunks)packages/discord.js/typings/index.test-d.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/discord.js/typings/index.d.ts (1)
packages/util/src/encodables.ts (1)
JSONEncodable(8-13)
🔇 Additional comments (3)
packages/discord.js/typings/index.d.ts (1)
6689-6701: BaseMessageOptions.components update matches “any JSONEncodable” direction.The switch to
ActionRowData<JSONEncodable<APIComponentInActionRow> | MessageActionRowComponentData>is consistent with dropping builder-specific component types while still supportingtoJSON()-based inputs.packages/discord.js/typings/index.test-d.ts (2)
4-20: Import paths correctly updated for builders module separation. ✓The builder imports are now correctly sourced directly from
@discordjs/buildersrather than through the discord.js package root, which aligns with the PR's objective to remove re-exports. All builder types and classes—includingActionRowBuilder,EmbedBuilder,MessageBuilder, and command builder types—are properly imported from the dedicated module.
826-827: Builder type declarations integrate correctly with test suite.The declarations of
slashCommandBuilderandcontextMenuCommandBuilderas types (not instances) are well-positioned and used consistently throughout the test suite (e.g., lines 847–855 for command creation/edit tests). These type declarations maintain the test's ability to validate discord.js's type compatibility with builder instances.
didinele
changed the title
didinele
force-pushed
the
refactor/remove-reexport
branch
from
39d6f3c to
aea7f94
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/discord.js/package.json (1)
83-86:⚠️ Potential issue | 🔴 CriticalPublic typings must not depend on a dev-only package.
Line 85 keeps
@discordjs/buildersindevDependencies. Ifpackages/discord.js/typings/index.d.ts(or.d.mts) still imports from@discordjs/builders, downstream TS users can fail module resolution.Suggested fix
"dependencies": { + "@discordjs/builders": "workspace:^", "@discordjs/collection": "workspace:^", "@discordjs/formatters": "workspace:^", "@discordjs/rest": "workspace:^", "@discordjs/util": "workspace:^", "@discordjs/ws": "workspace:^", @@ "devDependencies": { "@discordjs/api-extractor": "workspace:^", - "@discordjs/builders": "workspace:^", "@discordjs/docgen": "workspace:^",#!/bin/bash set -euo pipefail echo "== Check public typings for `@discordjs/builders` references ==" rg -nP '(`@discordjs/builders`)' \ packages/discord.js/typings/index.d.ts \ packages/discord.js/typings/index.d.mts || true echo echo "== Check where `@discordjs/builders` is declared ==" python - <<'PY' import json from pathlib import Path pkg = json.loads(Path("packages/discord.js/package.json").read_text()) for section in ("dependencies", "peerDependencies", "devDependencies"): print(f"{section}: {pkg.get(section, {}).get('@discordjs/builders')}") PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/discord.js/package.json` around lines 83 - 86, The package exposes typings that import `@discordjs/builders` (see packages/discord.js/typings/index.d.ts / index.d.mts) but `@discordjs/builders` is currently only in devDependencies; move `@discordjs/builders` out of devDependencies into either dependencies or peerDependencies in package.json so downstream TypeScript resolution succeeds (use peerDependencies if you expect consumers to provide it, dependencies if you want to bundle it). After updating package.json, verify there are no remaining references with the provided ripgrep/python checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/discord.js/package.json`:
- Around line 83-86: The package exposes typings that import `@discordjs/builders`
(see packages/discord.js/typings/index.d.ts / index.d.mts) but
`@discordjs/builders` is currently only in devDependencies; move
`@discordjs/builders` out of devDependencies into either dependencies or
peerDependencies in package.json so downstream TypeScript resolution succeeds
(use peerDependencies if you expect consumers to provide it, dependencies if you
want to bundle it). After updating package.json, verify there are no remaining
references with the provided ripgrep/python checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d2d7e48e-0144-441e-a00b-b19215f91f02
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
packages/builders/package.jsonpackages/builders/src/interactions/commands/chatInput/mixins/ApplicationCommandOptionChannelTypesMixin.tspackages/core/package.jsonpackages/discord.js/package.jsonpackages/discord.js/src/index.jspackages/discord.js/typings/index.d.tspackages/discord.js/typings/index.test-d.tspackages/formatters/package.jsonpackages/next/package.jsonpackages/rest/package.jsonpackages/structures/package.jsonpackages/util/package.jsonpackages/voice/package.jsonpackages/ws/package.json
💤 Files with no reviewable changes (1)
- packages/discord.js/src/index.js
📜 Review details
🔇 Additional comments (14)
packages/structures/package.json (1)
66-66: LGTM: dependency bump is consistent with the PR scope.This
discord-api-typespatch bump is aligned and safe at the manifest level.packages/core/package.json (1)
73-73: LGTM: clean patch-level dependency update.No issues identified with this package manifest change.
packages/util/package.json (1)
65-65: LGTM: manifest change looks correct.The
discord-api-typesbump is coherent with the rest of the PR.packages/next/package.json (1)
75-75: LGTM: dependency version bump is appropriate.No concerns with this isolated change.
packages/rest/package.json (1)
91-91: LGTM: this dependency bump is well-scoped.No manifest or dependency declaration issues spotted here.
packages/voice/package.json (1)
68-68: LGTM: dependency alignment looks good.This package.json change is consistent with the PR-wide version bump.
packages/ws/package.json (1)
81-81: LGTM: package manifest update is correct.No issues found in this dependency version increment.
packages/builders/package.json (1)
69-69: LGTM: dependency bump is consistent and safe.No additional action needed for this segment.
packages/formatters/package.json (1)
58-58: No actionable feedback for this segment.packages/discord.js/package.json (1)
76-76: No actionable feedback for this segment.packages/builders/src/interactions/commands/chatInput/mixins/ApplicationCommandOptionChannelTypesMixin.ts (1)
1-5: No actionable feedback for this segment.Also applies to: 19-19
packages/discord.js/typings/index.d.ts (2)
101-101: Good decoupling of published typings from builders.Line 101 correctly sources
ApplicationCommandOptionAllowedChannelTypefromdiscord-api-types/v10, removing the@discordjs/builderstyping dependency from this public declaration file.
6748-6748: Typing update matches the “JSONEncodable, builders-optional” direction.Line 6748 broadens
BaseMessageOptions.componentstoJSONEncodable<APIComponentInActionRow> | MessageActionRowComponentData, which preserves compatibility for builder-like/customtoJSON()implementations without requiring builders re-exports.packages/discord.js/typings/index.test-d.ts (1)
4-20: Direct builders import alignment looks correct.This import shift to
@discordjs/buildersis consistent with the re-export removal objective, and the imported symbols are used coherently throughout the test file.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/discord.js/typings/index.d.ts`:
- Line 6748: The ActionRowData generic currently allows
JSONEncodable<APIComponentInActionRow>, which includes modal-only components;
narrow it to message-only components by replacing
JSONEncodable<APIComponentInActionRow> with a JSONEncodable type limited to
message components (e.g. JSONEncodable<APIButtonComponent |
APISelectMenuComponent> or an existing alias representing message components) so
that the union becomes ActionRowData<JSONEncodable<APIButtonComponent |
APISelectMenuComponent> | MessageActionRowComponentData>; update the type where
BaseMessageOptions.components and the symbol ActionRowData are used and ensure
you reference APIComponentInActionRow, APIButtonComponent,
APISelectMenuComponent, and MessageActionRowComponentData when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bf49572a-5cf7-4770-bd1b-1e75f7ec071c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
packages/builders/package.jsonpackages/builders/src/interactions/commands/chatInput/mixins/ApplicationCommandOptionChannelTypesMixin.tspackages/core/package.jsonpackages/discord.js/package.jsonpackages/discord.js/src/index.jspackages/discord.js/typings/index.d.tspackages/discord.js/typings/index.test-d.tspackages/formatters/package.jsonpackages/next/package.jsonpackages/rest/package.jsonpackages/structures/package.jsonpackages/util/package.jsonpackages/voice/package.jsonpackages/ws/package.json
💤 Files with no reviewable changes (1)
- packages/discord.js/src/index.js
📜 Review details
🔇 Additional comments (14)
packages/structures/package.json (1)
66-66: Dependency update is aligned and safe in this manifest segment.Bumping
discord-api-typesto^0.38.41here matches the PR’s package-alignment goal and looks correct.packages/rest/package.json (1)
91-91: Dependency bump looks correct and scoped.Line 91 updates
discord-api-typesto^0.38.41, which matches the stated PR requirement and keeps this package aligned with the cross-repo API types update.packages/util/package.json (1)
65-65: Dependency bump is consistent and safe in this manifest segment.This update aligns with the PR-wide
discord-api-typesrefresh and does not introduce package.json structural issues.packages/voice/package.json (1)
68-68: LGTM! The patch-level bump todiscord-api-types^0.38.41 is valid and published. This aligns with the coordinated dependency update across the monorepo.packages/core/package.json (1)
73-73: Alldiscord-api-typesdeclarations across the workspace are consistently set to^0.38.41. No version skew detected.packages/next/package.json (1)
75-75: Dependency bump verified; versiondiscord-api-types@0.38.41exists on npm registry.No issues found. The version is published and resolvable. Change is safe to merge.
packages/ws/package.json (1)
81-81: Dependency bump is consistent with the coordinated workspace upgrade.No issues found in this segment.
packages/formatters/package.json (1)
58-58:discord-api-typesbump looks correct and aligned with the monorepo update.No blocking concerns here.
packages/builders/package.json (1)
69-69: Version update is coherent with the rest of the dependency alignment.This segment looks good.
packages/discord.js/package.json (2)
76-76:discord-api-typesbump is consistent with the coordinated package updates.No issues on this change.
85-85: Keeping@discordjs/buildersas a dev dependency is consistent with this PR’s API-surface change.Given the re-export removal and updated test/import strategy, this placement is coherent.
packages/builders/src/interactions/commands/chatInput/mixins/ApplicationCommandOptionChannelTypesMixin.ts (1)
1-5: Type-source and constraint update is clean and type-safe.The direct type import plus
satisfieskeeps the runtime list and API channel-type contract in sync.Also applies to: 19-19
packages/discord.js/typings/index.test-d.ts (1)
4-20: Typings test imports are correctly redirected to@discordjs/builders.This aligns the test surface with the removed re-exports from
discord.js.packages/discord.js/typings/index.d.ts (1)
101-101: Import decoupling from builders looks correct.Line 101 now sources
ApplicationCommandOptionAllowedChannelTypefromdiscord-api-types/v10, which matches the builders re-export removal goal.
Qjuh
left a comment
Qjuh
left a comment
There was a problem hiding this comment.
Apart from the nit pointed out by coderabbit LGTM
5 participants
people are gonna love me for this oneWe've glossed over this briefly in internals, and #11327 also mentioned that this is now possible. The primary argument here is just that there's absolutely no internal dependency on builders anymore (my PR leaves it as a devDep since it is used in tests to ensure things correctly accept
JSONEncodables of the right type), and this actually simplifies usage in a sense. There's no real versioning issues anymore, the major of builders you're on (or even implementation? anyone can write a class that implementsJSONEncodable, which discord.js will happily accept, it just needs to have atoJSONmethod) is not significant.We're also generally somewhat discouraging builders, it should be an option, but it's more of an extension that you can opt into.
"But we still re-export all the other packages" yeah. We critically depend on those for internals and version miss-matches there from a user also installing them can be pretty damaging.
On the topic of formatters, I'm not dying on the hill that we can't re-export them, but I don't necessarily see why we should for the same rationale: it's not a core dependency. We do depend on it to implement certain methods in internals, but that's ok.
Note: Requires discord-api-types bump.