Skip to content
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

Feat/add spindle hooks pkg #359

Merged
merged 9 commits into from
Apr 26, 2022
Merged

Feat/add spindle hooks pkg #359

merged 9 commits into from
Apr 26, 2022

Conversation

keiya01
Copy link
Contributor

@keiya01 keiya01 commented Mar 11, 2022

@openameba/spindle-hooks のpackageを追加しました。
spindle-uiからコピーしてきたファイルの中から不要なものを削除しました。

@keiya01 keiya01 requested a review from herablog March 11, 2022 03:05
@keiya01 keiya01 self-assigned this Mar 11, 2022
packages/spindle-hooks/docs/design-doc.md Outdated Show resolved Hide resolved
packages/spindle-hooks/package.json Show resolved Hide resolved
"react": "^16.8.0 || ^17.0.0"
},
"dependencies": {},
"devDependencies": {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spindle-uiからコピーしたものの中から不要なものを削除しました
storybook, reg-suit, firebase, style周りのdepsを消しています

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useCarouselの動きを確認するときにstorybookとfirebaseはあった方が良さそうなので入れちゃいます
reg-suitは迷っていますが動きの確認が中心になるのでなくても良いかなと思っています

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

b1f24c9
storybook追加してみました
設定はほとんどspindle-uiと同じです

@@ -0,0 +1,21 @@
MIT License
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

licenseはspindle-uiのままです

"files": [
{
"path": "./dist/**/*.js",
"maxSize": "1 kB"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

仮で1kbにしています

@@ -0,0 +1,57 @@
# Spindle Hooks (In development)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

別途改善します

@keiya01
Copy link
Contributor Author

keiya01 commented Mar 11, 2022

#175 を参考にESM考慮した方が良さそう

@keiya01 keiya01 force-pushed the feat/add-spindle-hooks-pkg branch 2 times, most recently from 831a58f to 3dd5083 Compare March 14, 2022 06:21
@@ -0,0 +1,5 @@
import { useSample } from './useSample';

test('useSample', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

Comment on lines 4 to 5
"main": "./dist/index.mjs",
"exports": "./dist/index.mjs",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cjsはないので"module"と区別していません。
exportsは指定されたパス以外でimportできないように制限できるようです:eyes:
https://shisama.hatenablog.com/entry/2020/12/21/090000

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

確かにhooksの新しいパッケージだしesm onlyで配信しても良さそうっすね。

あとこの辺も参考にやっておいた方がいいのかな.
https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c#how-can-i-make-my-typescript-project-output-esm

Copy link
Contributor Author

@keiya01 keiya01 Mar 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a3d982d
一応enginesだけ設定しました:pray:
"type": "module"はjestが動かないので設定していないです

@github-actions
Copy link
Contributor

github-actions bot commented Mar 28, 2022

Visit the preview URL for this PR (updated for commit 4d0e04d):

https://ameba-spindle-hooks--pr359-feat-add-spindle-hoo-7ed0ffcm.web.app

(expires Sun, 15 May 2022 09:04:44 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@keiya01
Copy link
Contributor Author

keiya01 commented Mar 29, 2022

https://ameba-spindle-hooks--pr359-feat-add-spindle-hoo-7ed0ffcm.web.app/
ameba-spindle-hooksにpreview反映されました

Copy link
Member

@herablog herablog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

エンジニアを褒めるネコ:HelloWorld

@keiya01
Copy link
Contributor Author

keiya01 commented Mar 31, 2022

commitまとめたのでprereleaseしてみます

@keiya01 keiya01 force-pushed the feat/add-spindle-hooks-pkg branch 3 times, most recently from c186003 to 17c7221 Compare March 31, 2022 07:49
@reg-suit
Copy link

reg-suit bot commented Mar 31, 2022

✨✨ That's perfect, there is no visual difference! ✨✨

Check out the report here.

@keiya01 keiya01 force-pushed the feat/add-spindle-hooks-pkg branch 5 times, most recently from ea51831 to 28eb9df Compare April 5, 2022 02:35
@keiya01 keiya01 force-pushed the feat/add-spindle-hooks-pkg branch 6 times, most recently from 23c6b5a to a1c93fc Compare April 12, 2022 04:33
@keiya01 keiya01 marked this pull request as draft April 12, 2022 07:11
@keiya01 keiya01 force-pushed the feat/add-spindle-hooks-pkg branch 9 times, most recently from 82c3112 to 4f28736 Compare April 15, 2022 08:30
keiya01 and others added 5 commits April 15, 2022 17:34
 - @openameba/spindle-hooks@0.0.1-alpha.0
 - @openameba/spindle-hooks@0.0.1-alpha.1
 - @openameba/spindle-hooks@0.0.1-alpha.2
openameba and others added 4 commits April 15, 2022 17:39
@keiya01 keiya01 marked this pull request as ready for review April 18, 2022 09:47
Comment on lines +22 to +28
"prebuild": "yarn clean",
"build": "run-s build:esm build:cjs",
"build:cjs": "tsc -p tsconfig.cjs.json",
"build:esm": "run-s build:esm:js build:esm:rename",
"build:esm:js": "tsc -p tsconfig.esm.json",
"build:esm:rename": "npx renamer --find js --replace mjs 'dist/**'",
"prepublishOnly": "yarn build && yarn cp"
Copy link
Contributor Author

@keiya01 keiya01 Apr 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@herablog
この辺りだけspindle-uiに合わせてbundle作るように修正したのでざっと確認お願いします:pray:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q. そういえば配信方法変更したのは、どこがうまく動かなかったんだっけか..!

Copy link
Contributor Author

@keiya01 keiya01 Apr 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Storybookがうまく動かずで変更しました!
type: "module" (内部でimport { Hoge } from "./mod.js"みたいに読み込んでいるケース) の場合にESMが動かなかっただけなのでmjsだけ配信して、cjsは配信しなくても良いかもです

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

こりと同じ問題て感じかな..!?
storybookjs/storybook#9854 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

それです!リンク貼り忘れてましたすみません、、:pray:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

いえいえ〜そのうち対応進みそうな気もするけど、タイミングがむずかしいね:pien_2:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

難しいですね、、
他にもjestとかもまだexperimentalだったりするのでもあと少しという感じですかね、、

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ですよね〜いろんなアプリケーションから使われる性質を考えると、修正してもらった方針でいきましょうか!

@keiya01
Copy link
Contributor Author

keiya01 commented Apr 18, 2022

[MEMO]
このPRマージ後にuseTimeDistance追加する

Copy link
Member

@herablog herablog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

エンジニアを褒めるネコ:リリースしました

@keiya01 keiya01 merged commit 0b05e5a into main Apr 26, 2022
@keiya01 keiya01 deleted the feat/add-spindle-hooks-pkg branch April 26, 2022 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants