token json を結合して color や数値をとれるトークンオブジェクトを作成する #639
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
naporin0624
had a problem deploying
to
preview-channel
GitHub Actions
Failure
naporin0624
force-pushed
the
feta/create-styled-tokens
branch
from
f3bf240
to
b28a6a3
Compare
naporin0624
temporarily deployed
to
preview-channel
GitHub Actions
Inactive
|
Visit the preview URL for this PR (updated for commit 001e909): https://pixiv-charcoal-web--pr639-feta-create-styled-t-9ot7uuut.web.app (expires Fri, 01 Nov 2024 17:16:23 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 314b26d3adca98a761c7e4d9922ebb206ff024a0 |
|
Size Change: +13.5 kB (+0.05%) Total Size: 26 MB
ℹ️ View Unchanged
|
naporin0624
temporarily deployed
to
preview-channel
GitHub Actions
Inactive
naporin0624
temporarily deployed
to
preview-channel
GitHub Actions
Inactive
naporin0624
force-pushed
the
feta/create-styled-tokens
branch
from
651471b
to
356f180
Compare
naporin0624
temporarily deployed
to
preview-channel
GitHub Actions
Inactive
naporin0624
temporarily deployed
to
preview-channel
GitHub Actions
Inactive
naporin0624
had a problem deploying
to
preview-channel
GitHub Actions
Failure
naporin0624
temporarily deployed
to
preview-channel
GitHub Actions
Inactive
naporin0624
changed the title
naporin0624
commented
Oct 23, 2024
| @@ -37,6 +37,8 @@ | |||
| "dependencies": { | |||
| "@charcoal-ui/foundation": "^4.0.0-beta.15", | |||
| "@charcoal-ui/utils": "^4.0.0-beta.15", | |||
| "change-case-all": "^2.1.0", | |||
There was a problem hiding this comment.
change-case が esm only で commonjs build できないので対応しているほうを導入
github の star は少ないが @graphql/xxx にも使用されているので問題なしと判断
https://www.npmjs.com/package/change-case-all?activeTab=dependents
There was a problem hiding this comment.
割とesmがblockingになるケース最近あんまりない気がするけど、一旦change-case-allにしても大丈夫です。change-caseの若干(500Bくらい)小さいです
There was a problem hiding this comment.
使う側はそうなんですが、@charcoal/theme 自体が dual pacakge build しているので create-token-object を package として export するなら esm, commonjs 対応の依存が必要になってくるというやつでした
naporin0624
commented
Oct 23, 2024
| import { toTokenObject } from './to-token-object' | ||
| import type { TokenObject, TokenDictionary, TokenValue } from './types' | ||
|
|
||
| export { camelCaseKeys } from './helpers/changecase-keys' |
There was a problem hiding this comment.
以下のように使って camelCase の token を作ることを想定。形式は snapshot test を参照
https://github.com/pixiv/charcoal/blob/02bb4dcd24a42d8b7f59daab7b703f518384db0b/packages/theme/src/token-object/helpers/__snapshots__/changecase-keys.test.ts.snap
const camelCaseTokne = cacelCaseKeys(createTokenObject(light, base))
naporin0624
commented
Oct 23, 2024
Comment on lines
+35
to
+38
| expect(theme).toHaveProperty( | ||
| [_category, ...splitted], | ||
| templateResolver(tokenValue.value) | ||
| ) |
There was a problem hiding this comment.
オブジェクトのネストが正しいかということだけしかチェックできていない。実際に値が正しいかのチェックは実装と同じ関数を使ってテストしてしまっているので効力がなさそうでちょっと困っている。
とはいえ確かめる方法がこれしか思い浮かばなかった.....
yue4u
reviewed
Oct 23, 2024
There was a problem hiding this comment.
見ました！
createTokenObjectとcreateReferenceTokenResolverの実装が良さそうです。テストとても助かります。
検討したいこと
- 事前にcreateTokenObject+camelcaseの結果作って配ったほうがruntime側のコストが低いかも
- jsonはReference済みの非camelcaseの同じ内容にしてみたい
- 実際今style dictionary形式のjsonを直接扱える手段がない
- 今の型はtypescriptのjsonサポート頼りで、不安定になる可能性がありつつ一旦は動く
- themeサポートの意味では中身がcss変数のほうが嬉しいかも
| @@ -37,6 +37,8 @@ | |||
| "dependencies": { | |||
| "@charcoal-ui/foundation": "^4.0.0-beta.15", | |||
| "@charcoal-ui/utils": "^4.0.0-beta.15", | |||
| "change-case-all": "^2.1.0", | |||
There was a problem hiding this comment.
割とesmがblockingになるケース最近あんまりない気がするけど、一旦change-case-allにしても大丈夫です。change-caseの若干(500Bくらい)小さいです
| @@ -37,6 +37,8 @@ | |||
| "dependencies": { | |||
| "@charcoal-ui/foundation": "^4.0.0-beta.15", | |||
| "@charcoal-ui/utils": "^4.0.0-beta.15", | |||
| "change-case-all": "^2.1.0", | |||
| "deepmerge": "^4.3.1", | |||
There was a problem hiding this comment.
二つのライブラリーもruntimeに入れるくらいなら事前に作ったものと実際比較してみたいです
(現在両方+2kBくらい)
また、dependenciesにあるとビルドされても使う側でもう一度installされっるのでdevDependenciesに移動したいかもしれないです
There was a problem hiding this comment.
メモ: こういう形にすると一つ4KB gzip (12KB before)
There was a problem hiding this comment.
css variables のものと camelcase のもの, json の key そのままのものを複数用意しました。すべて dist/json に吐き出されるようにしています
packages/theme/src/index.ts
Outdated
| @@ -1,3 +1,4 @@ | |||
| export * from './theme' | |||
| export * from './abstract-theme' | |||
| export * from './default' | |||
| export * from './token-object' | |||
There was a problem hiding this comment.
ここでre-exportしたらindexにbundleされそうので、entry point分けた別ファイルでもいいかもしれないです
There was a problem hiding this comment.
https://github.com/pixiv/charcoal/blob/feta/create-styled-tokens/packages/theme/package.json#L21
dist が files に入っていて src/css と src/json を dist に copy するようになってるのでそこは問題ないかなという感じです
There was a problem hiding this comment.
いいえ、distにあっても"exports"に足さないとbundlerによってimportすることを許されない気がします。
There was a problem hiding this comment.
以下を足すと少し良くなります
"./json/*": "./dist/json/*",
"./css/*": "./dist/css/*"
There was a problem hiding this comment.
いいえ、distにあっても"exports"に足さないとbundlerによってimportすることを許されない気がします。
勘違いしていました。そうでしたね。修正します
Comment on lines
+4
to
+6
| import lightToken from '../../json/pixiv-light.json' | ||
| import darkToken from '../../json/pixiv-dark.json' | ||
| import baseToken from '../../json/base.json' |
There was a problem hiding this comment.
メモ: json頼りで型生成するには制限があって、一定以上のサイズ超えるると推論してくれません
| ['light theme', lightToken], | ||
| ['dark theme', darkToken], | ||
| ] as const)('createTokenObject test: %s', (description, token) => { | ||
| const theme = createTokenObject(token, baseToken) |
There was a problem hiding this comment.
メモ: createTokenObjectは 7~10msくらいかかるのでややCPU heavy。dark+light 両方作るなら20msになるのでやはり事前で作ったほうが嬉しい気がしました
There was a problem hiding this comment.
こういうのは bench 取るようにします:pray: camel case 変換まで入れるとだいぶ遅いので static json として out したほうがいいですね
✓ src/token-object/token-object.bench.ts (6) 3716ms
✓ createTokenObject test: light theme (3) 1870ms
name hz min max mean p75 p99 p995 p999 rme samples
· benchmarks token object creation for the theme 209.88 4.1708 8.3277 4.7646 4.7636 8.0440 8.3277 8.3277 ±2.82% 105 fastest
· benchmarks token object creation with camelCase formatting 188.30 4.5044 9.4341 5.3108 5.3794 9.4341 9.4341 9.4341 ±3.91% 95 slowest
· benchmarks css token object creation 191.89 4.7681 6.9201 5.2112 5.3368 6.9201 6.9201 6.9201 ±1.17% 96
✓ createTokenObject test: dark theme (3) 1843ms
name hz min max mean p75 p99 p995 p999 rme samples
· benchmarks token object creation for the theme 226.93 4.0968 5.2015 4.4066 4.5096 4.8796 5.2015 5.2015 ±0.78% 114 fastest
· benchmarks token object creation with camelCase formatting 212.90 4.3832 5.3154 4.6971 4.8030 5.1833 5.3154 5.3154 ±0.71% 107
· benchmarks css token object creation 189.43 4.7522 8.2693 5.2789 5.2970 8.2693 8.2693 8.2693 ±2.43% 95 slowest
naporin0624
commented
Oct 24, 2024
packages/theme/package.json
Outdated
Comment on lines
9
to
20
| ".": { | ||
| "types": "./dist/index.d.ts", | ||
| "require": "./dist/index.cjs.js", | ||
| "import": "./dist/index.esm.js", | ||
| "default": "./dist/index.esm.js" | ||
| }, | ||
| "./token-object": { | ||
| "types": "./dist/token-object/index.d.ts", | ||
| "require": "./dist/token-object/index.cjs.js", | ||
| "import": "./dist/token-object/index.esm.js", | ||
| "default": "./dist/token-object/index.esm.js" | ||
| } |
There was a problem hiding this comment.
token-object だけ subpath に分けました。
naporin0624
commented
Oct 24, 2024
packages/theme/package.json
Outdated
Comment on lines
38
to
46
| "token-object": "npm-run-all --print-label --parallel 'token-object:*'", | ||
| "token-object:light": "npm-run-all --print-label --parallel 'token-object:light:*'", | ||
| "token-object:light:default": "tsx cli/token-object --token ./src/json/pixiv-light.json --base ./src/json/base.json --output ./dist/json/pixiv-light.json", | ||
| "token-object:light:camel": "tsx cli/token-object --token ./src/json/pixiv-light.json --base ./src/json/base.json --output ./dist/json/camel/pixiv-light.json --format camel", | ||
| "token-object:light:css": "tsx cli/token-object --token ./src/json/pixiv-light.json --base ./src/json/base.json --output ./dist/json/css/pixiv-light.json --format css", | ||
| "token-object:dark": "npm-run-all --print-label --parallel 'token-object:dark:*'", | ||
| "token-object:dark:default": "tsx cli/token-object --token ./src/json/pixiv-dark.json --base ./src/json/base.json --output ./dist/json/pixiv-dark.json", | ||
| "token-object:dark:camel": "tsx cli/token-object --token ./src/json/pixiv-dark.json --base ./src/json/base.json --output ./dist/json/camel/pixiv-dark.json --format camel", | ||
| "token-object:dark:css": "tsx cli/token-object --token ./src/json/pixiv-dark.json --base ./src/json/base.json --output ./dist/json/css/pixiv-dark.json --format css" |
There was a problem hiding this comment.
build 時に dist/json 以下に変換済みのものを吐き出すようにしました
There was a problem hiding this comment.
良いディレクトリ名が思いつかなかったので良いものがあれば変えたいです
There was a problem hiding this comment.
/tokensでもいいと思いますがtoken-objectでもそこまで違和感ないです。 @mimokmt どうでしょう
There was a problem hiding this comment.
pureなjsから動作確認 ✅
import light from "@charcoal-ui/theme/dist/json/css/pixiv-light.json" with { type: "json" };
import dark from "@charcoal-ui/theme/dist/json/camel/pixiv-dark.json" with { type: "json" };
console.log(light["border-width"].l);
console.log(dark.borderWidth.l);細かいですが、個人的な感想として
- distがpathに含まれるとimportしていいのか少し戸惑う
- cssよりvariablesかcss-variablesのほうが伝わりそう
yue4u
reviewed
Oct 25, 2024
packages/theme/package.json
Outdated
Comment on lines
38
to
46
| "token-object": "npm-run-all --print-label --parallel 'token-object:*'", | ||
| "token-object:light": "npm-run-all --print-label --parallel 'token-object:light:*'", | ||
| "token-object:light:default": "tsx cli/token-object --token ./src/json/pixiv-light.json --base ./src/json/base.json --output ./dist/json/pixiv-light.json", | ||
| "token-object:light:camel": "tsx cli/token-object --token ./src/json/pixiv-light.json --base ./src/json/base.json --output ./dist/json/camel/pixiv-light.json --format camel", | ||
| "token-object:light:css": "tsx cli/token-object --token ./src/json/pixiv-light.json --base ./src/json/base.json --output ./dist/json/css/pixiv-light.json --format css", | ||
| "token-object:dark": "npm-run-all --print-label --parallel 'token-object:dark:*'", | ||
| "token-object:dark:default": "tsx cli/token-object --token ./src/json/pixiv-dark.json --base ./src/json/base.json --output ./dist/json/pixiv-dark.json", | ||
| "token-object:dark:camel": "tsx cli/token-object --token ./src/json/pixiv-dark.json --base ./src/json/base.json --output ./dist/json/camel/pixiv-dark.json --format camel", | ||
| "token-object:dark:css": "tsx cli/token-object --token ./src/json/pixiv-dark.json --base ./src/json/base.json --output ./dist/json/css/pixiv-dark.json --format css" |
There was a problem hiding this comment.
基本一緒に生成するのでここでコマンド増やすよりcli側でまとめたほうがメンテしやすい気がします。MUSTではないので一旦このままでokです
yue4u
reviewed
Oct 25, 2024
| "typescript": "^4.9.5", | ||
| "vitest": "^2.0.2" | ||
| }, | ||
| "dependencies": { | ||
| "@charcoal-ui/foundation": "^4.0.0-beta.15", | ||
| "@charcoal-ui/utils": "^4.0.0-beta.15", | ||
| "change-case-all": "^2.1.0", | ||
| "deepmerge": "^4.3.1", |
There was a problem hiding this comment.
こちらのdepsもうdevdependenciesに移動していい気がしました
There was a problem hiding this comment.
subpath で export している以上 dependencies には必要かなと思っていたんですが、devDependencies にして subpath による export 切っちゃったほうが良いですかね？
There was a problem hiding this comment.
ビルド時bundleされていれば不要と言おうとしていたがdepsはbundleされていないようですね。一旦現状でも大丈夫です
yue4u
approved these changes
Oct 25, 2024
naporin0624
temporarily deployed
to
preview-channel
GitHub Actions
Inactive
yue4u
approved these changes
Oct 30, 2024
Comment on lines
+88
to
+90
| const baseJson = JSON.parse( | ||
| readFileSync(path.resolve(baseFile), 'utf8') | ||
| ) as TokenDictionary |
There was a problem hiding this comment.
configurationごとreadFile + JSON.parseする必要がないのと、このくらいな量ならasync apiのほうがいいかもしれないですが、ここでそこまでパフォーマンス求めないので一旦これでも大丈夫です
time yarn build
> yarn build 5.07s user 0.49s system 152% cpu 3.645 total
| "cpx": "^1.5.0", | ||
| "css": "^3.0.0", |
There was a problem hiding this comment.
気づいてなかったでした。
https://www.npmjs.com/package/css メンテされていなさそうでpostcssかlightningcssに置き換えたらいいかもしれないですね
|
念の為確認です！
イメージに違いないでしょうか？ |
mimokmt
approved these changes
Oct 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
やったこと
動作確認環境
チェックリスト
不要なチェック項目は消して構いません