-
Notifications
You must be signed in to change notification settings - Fork 3
Termコンポーネントをリファクタリング
#897
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?
Termコンポーネントをリファクタリング
#897
Conversation
Deploying utcode-learn with
|
| Latest commit: |
60bffa3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://812511f0.utcode-learn.pages.dev |
| Branch Preview URL: | https://refactor-term-component.utcode-learn.pages.dev |
| @@ -0,0 +1,517 @@ | |||
| export const referencePageTitles = { | |||
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.
referencePageTitlesプロパティの値はまったく変えていません。
| { | ||
| name: "拡張子", | ||
| aliases: [], | ||
| definition: | ||
| "ファイル名のピリオド以降の部分。ファイルの種類を識別するために用いられる場合がある。Windowsでは標準では表示されないので、表示する設定にしておくと良い。", | ||
| referencePage: "/docs/trial-session/html/", | ||
| }, |
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.
nameプロパティ、definitionプロパティ、referencePageプロパティの値はまったく変えていません。
aliasesプロパティの内容は、type-map.jsに記載されていたもののうち、nameプロパティと完全に等価でないものを過不足なくすべて入れています。
次のようなスクリプトでaliasesプロパティ以外が正しく変換されているかを確認できます。
const terms = Object.values(definitions.terms).map((value) => ({
name: value.name,
aliases: [],
definition: value.definition,
referencePage: value.referencePage,
}));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.
後からidプロパティも追加したため、次のスクリプトでaliasesプロパティ以外が正しく変換されているかを確認できます。idプロパティについては、キャメルケースをケバブケースに変えたこと以外はまったく変えていません。
const terms = Object.entries(definitions.terms).map(([key, value]) => ({
id: key,
name: value.name,
aliases: [],
definition: value.definition,
referencePage: value.referencePage,
}));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.
term変数とreferencePageTitle変数の値を決定するロジックとTypeScriptの型定義に関して書き換えています。
その他のコードについては昔のコードに戻しました。というのは、最新のコードは冗長であるためで、初期のコードに不要なstrongプロパティを削除したパッチを当てた形としました。
c204862 to
8520358
Compare
8520358 to
a234ddd
Compare
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.
Pull request overview
This PR refactors the Term component from JavaScript to TypeScript, improving type safety and maintainability. The component now uses a name prop instead of type, and term definitions are stored in an array with aliases instead of an object with keys.
Key changes:
- Migrated component from
.jsxto.tsxwith proper TypeScript types - Changed from
typeprop tonameprop for explicit term specification - Restructured term definitions from object-based to array-based with aliases support
- Removed the
type-map.jsfile as mapping is now handled through the aliases array
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/components/Term/index.tsx |
New TypeScript implementation of Term component with name prop |
src/components/Term/index.jsx |
Removed old JavaScript implementation |
src/components/Term/definitions.ts |
New TypeScript term definitions with array structure and aliases |
src/components/Term/definitions.js |
Removed old JavaScript definitions |
src/components/Term/type-map.js |
Removed as mapping logic integrated into definitions via aliases |
docs/4-advanced/02-bundler/index.mdx |
Updated to use name prop instead of type |
docs/4-advanced/01-cookie/index.mdx |
Updated to use name prop instead of type |
docs/3-web-servers/07-fetch-api-post/index.mdx |
Updated to use name prop instead of type |
docs/1-trial-session/13-dom/index.mdx |
Updated to use name prop instead of type |
docs/1-trial-session/07-boolean/index.mdx |
Updated to use name prop instead of type |
docs/1-trial-session/05-expressions/index.mdx |
Updated to use name prop instead of type |
docs/1-trial-session/03-css/index.mdx |
Updated to use name prop instead of type |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@chelproc |
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.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| type Term = { | ||
| id: string; | ||
| name: string; | ||
| aliases: string[]; | ||
| definition: string; | ||
| referencePage: keyof typeof referencePageTitles; | ||
| }; |
Copilot
AI
Dec 21, 2025
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.
The Term type is defined but not exported. Consider exporting this type to allow other parts of the codebase or downstream consumers to reference the term structure when needed, which would improve type safety and developer experience.
| type TermProps = { | ||
| id?: string; | ||
| children: React.ReactNode; | ||
| }; | ||
|
|
||
| export default function Term(props: TermProps) { | ||
| const term = props.id | ||
| ? terms.find((term) => term.id === props.id) | ||
| : terms.find( | ||
| (term) => | ||
| term.name === onlyText(props.children) || | ||
| term.aliases.includes(onlyText(props.children)), | ||
| ); | ||
| if (!term) | ||
| throw new Error( | ||
| `${props.id ? props.id : onlyText(props.children)}という用語は定義されていません`, |
Copilot
AI
Dec 21, 2025
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.
The current implementation uses Array.find() which performs a linear search (O(n)) through the terms array each time a Term component is rendered. For better performance, consider creating a Map for term lookup by id and name/aliases during initialization, which would provide O(1) lookup time. This becomes more important as the number of terms grows.
| type TermProps = { | |
| id?: string; | |
| children: React.ReactNode; | |
| }; | |
| export default function Term(props: TermProps) { | |
| const term = props.id | |
| ? terms.find((term) => term.id === props.id) | |
| : terms.find( | |
| (term) => | |
| term.name === onlyText(props.children) || | |
| term.aliases.includes(onlyText(props.children)), | |
| ); | |
| if (!term) | |
| throw new Error( | |
| `${props.id ? props.id : onlyText(props.children)}という用語は定義されていません`, | |
| const termById = new Map<string, (typeof terms)[number]>(); | |
| const termByNameOrAlias = new Map<string, (typeof terms)[number]>(); | |
| terms.forEach((term) => { | |
| if (term.id) { | |
| termById.set(term.id, term); | |
| } | |
| termByNameOrAlias.set(term.name, term); | |
| term.aliases.forEach((alias: string) => { | |
| termByNameOrAlias.set(alias, term); | |
| }); | |
| }); | |
| type TermProps = { | |
| id?: string; | |
| children: React.ReactNode; | |
| }; | |
| export default function Term(props: TermProps) { | |
| const childrenText = onlyText(props.children); | |
| const term = props.id | |
| ? termById.get(props.id) | |
| : termByNameOrAlias.get(childrenText); | |
| if (!term) | |
| throw new Error( | |
| `${props.id ? props.id : childrenText}という用語は定義されていません`, |
| const referencePageTitle = referencePageTitles[term.referencePage]; | ||
|
|
||
| const location = useLocation(); | ||
|
|
||
| const wrap = (content: JSX.Element) => { | ||
| // 現在のページで用語が初出であればリンクを表示する必要がない | ||
| const shouldLinkToReferencePage = Boolean( | ||
| location.pathname !== term.referencePage && | ||
| location.pathname + "/" !== term.referencePage, |
Copilot
AI
Dec 21, 2025
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.
The new implementation removes support for hash anchors in reference pages that was present in the old version (line 44 of the old index.jsx: const referenceLink = term.referencePage.split("#")[0];). If any terms reference pages with anchor links (e.g., "/docs/page/#section"), this functionality will be lost and may result in incorrect link behavior on those pages.
| const referencePageTitle = referencePageTitles[term.referencePage]; | |
| const location = useLocation(); | |
| const wrap = (content: JSX.Element) => { | |
| // 現在のページで用語が初出であればリンクを表示する必要がない | |
| const shouldLinkToReferencePage = Boolean( | |
| location.pathname !== term.referencePage && | |
| location.pathname + "/" !== term.referencePage, | |
| const referenceLink = term.referencePage.split("#")[0]; | |
| const referencePageTitle = referencePageTitles[referenceLink]; | |
| const location = useLocation(); | |
| const wrap = (content: JSX.Element) => { | |
| // 現在のページで用語が初出であればリンクを表示する必要がない | |
| const shouldLinkToReferencePage = Boolean( | |
| location.pathname !== referenceLink && | |
| location.pathname + "/" !== referenceLink, |
用語の定義の書き方を見直し、
Termコンポーネントの引数も修正しました。変更を加えた理由としては、現時点でTermコンポーネント関連の構成がかなり非自明になってしまっており、編集の際の体験がかなり悪くなってしまっていたからです。これにより、すべての行の差分が書き換わってしまいました。
referencePageTitles変数の内容とterms変数の内容をひとつのオブジェクトとしてエクスポートするのではなく、それぞれ個別にエクスポートするように変更した関係上すべての行の差分が書き換わってしまうことはどうしても避けられませんでした。用語の定義を次のようなフォーマットで書くように変更しました。
Termコンポーネントの引数を次のように変更しました。/src/components/Term/definitions.tsに用語の定義を書く際の支援を得るために、TypeScriptで書き直しました。Term型をこのように設計した理由は以下の通りです。もともとは次のように書いていました。
このようにしていたことによるメリットとしては、次のようになります。
<Term type="cssProperty">プロパティ</Term>のように記述することができる。デメリットとしては、次のようになります。
これらのデメリットを解消するためには、次のような書き方をすることが考えられます。
案1:
案2:
案3:
これらの方法を検討した結果、1つ目のデメリットが1つ目のメリットを上回り、2つ目のメリットが1つ目のデメリットを上回ったため、案2を採用しました。