Feat: selector() API implement #236#246
Conversation
🦋 Changeset detectedLatest commit: d9445d5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Triggered from #246 by @black7375. Checking if we can fast forward Target branch ( commit aafcd1c4380d6778f7ee1864c08a56f46944ae91 (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Tue Jul 29 00:00:00 2025 +0900
Feat: `rules.with()` API implement #234Pull request ( commit d9445d541aae580053755bfc3a5f9e07620241d0 (pull_request/selector-util)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Thu Jul 31 00:00:00 2025 +0900
Feat: `selector()` API implement #236It is possible to fast forward |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a new selector() utility function to improve TypeScript type inference for computed property names in CSS objects, addressing issue #236.
- Added a
selector()utility function that accepts a string and returns it typed as& - Exported the new function from main package entry points
- Updated changeset to reflect the new feature addition
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/css/src/css/index.ts | Implements the core selector() utility function |
| packages/css/src/index.ts | Exports the new selector function from main entry point |
| packages/css/src/compat.ts | Exports selector for compatibility |
| .changeset/strong-cases-grin.md | Updates version bump from patch to minor |
| .changeset/great-buckets-clean.md | Adds changeset documenting the new feature |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| export function selector(selector: string): `&` { |
There was a problem hiding this comment.
The function returns a template literal type & but accepts any string. This could lead to runtime issues if the selector doesn't actually contain &. Consider adding validation or documenting that the input should contain & for proper functionality.
| export function selector(selector: string): `&` { | |
| /** | |
| * Accepts a selector string that must contain '&' and returns it as a template literal type. | |
| * Throws an error if the selector does not contain '&'. | |
| */ | |
| export function selector(selector: string): `&` { | |
| if (!selector.includes('&')) { | |
| throw new Error("Selector must contain '&'. Received: " + selector); | |
| } |
| export function mincho$<T>(block: () => T) { | ||
| return block(); | ||
| } | ||
|
|
There was a problem hiding this comment.
The selector function lacks JSDoc documentation. Consider adding documentation to explain its purpose, expected input format, and usage example as shown in the PR description.
| /** | |
| * Marks a string as a selector reference for use in style definitions. | |
| * | |
| * @param selector - The selector string to be marked as a reference (e.g., "&:hover"). | |
| * @returns The selector string, typed as `&`, for use in style rules. | |
| * | |
| * @example | |
| * ```ts | |
| * const style = { | |
| * [selector("&:hover")]: { color: "red" } | |
| * }; | |
| * ``` | |
| */ |
|
@coderabbitai fullreview |
|
@black7375: I'll conduct a full review of the PR changes now. ✅ Actions performedFull review triggered. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new public API utility Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Consumer as Consumer Code
participant Pkg as @mincho-js/css (re-exports)
participant Impl as css/index.ts (implementation)
Consumer->>Pkg: import { selector }
Pkg->>Impl: resolve re-export -> selector
Consumer->>Impl: selector(":hover &")
note over Impl: Treat input as typed selector (returns '&' literal-typed string)
Impl-->>Consumer: typed selector string
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
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 |
|
@coderabbitai summary |
✅ Actions performedSummary regeneration 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.changeset/strong-cases-grin.md (1)
1-8: Changeset describes unrelated feature.This changeset documents
rules.multiple()API, but the PR implementsselector()utility. The content does not match the changes in this PR.Please ensure this changeset either:
- Documents the
selector()feature (if this is the intended changeset for this PR), or- Is removed from this PR if it belongs to a different feature branch
🧹 Nitpick comments (2)
.changeset/great-buckets-clean.md (1)
1-9: LGTM! Changeset correctly documents the new feature.The changeset properly documents the
selector()utility addition with appropriate version bump (minor) and clear description.Note: There's a minor markdown linting issue on line 5 (emphasis instead of heading), but this is a pedantic style concern and can be addressed optionally.
If you want to address the linting issue, change line 5 from:
-**css** +## csspackages/css/src/css/index.ts (1)
242-244: Consider adding tests for the new function.The file includes comprehensive tests for other functions, but the new
selector()utility has no test coverage.Add test coverage to verify:
- The function returns the input string unchanged
- TypeScript correctly infers the
&literal type- Integration with
css()for computed property names (as shown in PR example)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/great-buckets-clean.md(1 hunks).changeset/strong-cases-grin.md(1 hunks)packages/css/src/compat.ts(1 hunks)packages/css/src/css/index.ts(1 hunks)packages/css/src/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-09T13:00:38.449Z
Learnt from: black7375
PR: mincho-js/mincho#113
File: packages/css/src/index.ts:33-33
Timestamp: 2024-10-09T13:00:38.449Z
Learning: `recipe` is properly exported from './rules' in `packages/css/src/rules/index.ts`.
Applied to files:
packages/css/src/index.ts
🪛 markdownlint-cli2 (0.18.1)
.changeset/great-buckets-clean.md
5-5: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (3)
packages/css/src/css/index.ts (1)
242-244: Consider whether runtime validation is needed.A past review comment suggested validating that the selector contains
&. However, since this appears to be a type-level utility for improving TypeScript inference (not runtime behavior), validation may not be necessary.Please clarify:
- Is this function intended purely for type inference, or does it affect runtime behavior?
- Should invalid selectors fail at compile-time, runtime, or is flexibility preferred?
The current implementation (simple type cast) is appropriate for a type utility, but if runtime validation is desired, add the check suggested in the past review.
packages/css/src/index.ts (1)
28-28: LGTM! Export correctly added.The
selectorexport is properly added alongside related CSS utilities, maintaining consistency with the existing export pattern.packages/css/src/compat.ts (1)
24-26: LGTM! Compat export correctly added.The
selectorexport is properly added to the compat entry point. Unlike other exports that provide aliased names (globalCss→globalStyle,css→style),selectoris exported with its original name, which is appropriate for a new API.
| export function selector(selector: string): `&` { | ||
| return selector as `&`; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add JSDoc documentation for public API.
The selector() function is part of the public API but lacks documentation explaining its purpose, usage, and expected input format.
Add documentation like:
+/**
+ * Type utility that marks a selector string for use as a computed property name.
+ * Enables proper TypeScript inference when using selectors as object keys.
+ *
+ * @param selector - A CSS selector string (e.g., ".otherClass &", "&:hover")
+ * @returns The selector string, typed as `&` for TypeScript inference
+ *
+ * @example
+ * ```ts
+ * css({
+ * [selector(".otherClass &")]: {
+ * color: "red"
+ * }
+ * });
+ * ```
+ */
export function selector(selector: string): `&` {
return selector as `&`;
}🤖 Prompt for AI Agents
In packages/css/src/css/index.ts around lines 242 to 244, the public API
function selector lacks JSDoc; add a concise JSDoc block immediately above the
function that explains its purpose (marks a string as a nested selector
reference `&` for use in css() objects), describes the expected input format (a
selector string that contains `&`, e.g. ".otherClass &"), documents the
parameter and return type, and includes a short usage example showing css({
[selector(".otherClass &")]: { color: "red" } }); so consumers understand how to
use it.
|
/fast-forward |
|
Triggered from #246 (comment) by @black7375. Trying to fast forward Target branch ( commit aafcd1c4380d6778f7ee1864c08a56f46944ae91 (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Tue Jul 29 00:00:00 2025 +0900
Feat: `rules.with()` API implement #234Pull request ( commit d9445d541aae580053755bfc3a5f9e07620241d0 (pull_request/selector-util)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Thu Jul 31 00:00:00 2025 +0900
Feat: `selector()` API implement #236Fast forwarding $ git push origin d9445d541aae580053755bfc3a5f9e07620241d0:main
To https://github.com/mincho-js/mincho.git
aafcd1c..d9445d5 d9445d541aae580053755bfc3a5f9e07620241d0 -> main |
Description
Type inference doesn't work well for computed property names in TypeScript.
This type utility makes it easier to handle this.
Related Issue
Summary by CodeRabbit
New Features
Documentation
Chores
Additional context
Checklist