StoreName type check not depends on infer #10

AkifumiSato · 2023-09-12T12:43:55Z

誤ったstoreName指定時にtypecheckでfailするよう修正。

koichik · 2023-09-12T14:27:47Z

これストア名を間違えた場合に型エラーになる？手元ではならないけれど。。。？？

koichik

難しい && 悩ましい

packages/location-state-core/src/types.ts

koichik · 2023-09-12T14:31:44Z

packages/location-state-core/src/hooks.ts

  name: string;
  defaultValue: T;
-  storeName: StoreName | string;
+  // Avoid inference to rigorously check type arguments
+  storeName: StoreName extends infer S ? S : never;


これがうまくいくにはStoreNameのデフォルトがQiitaの記事のようにneverでないとダメなのでは？
string extends infer SだとSはstringをキャプチャしてしまうからneverにはならないと思われ

LocationStateDefinition 単体ではこれで推論されることはなくなる＋明示的に型引数で指定するとうまくいってました。
Qiitaの記事は型引数を必ず指定することを目的として推論させない＋neverデフォルトなアプローチであって、推論されることを防ぐのみならStoreName extends infer S ? S : never;でOKだからうまくいったんだと思ってました。

LocationStateDefinition単体だとstoreName: StoreNameでもエラーになっていたはず
つまり型変数StoreNameが普通にDefaultStoreName(s)になるんでしょう
関数の型推論が特別に強力なんだろうなぁ

あ、確かにでした。
関数の推論をStoreName extends infer S ? S : never;で防げるからうまくいくってところですかね...？

この修正もう不要ですよね？
storeName: StoreNameでいいはず

packages/location-state-core/src/hooks.ts

AkifumiSato · 2023-09-12T14:47:07Z

これストア名を間違えた場合に型エラーになる？手元ではならないけれど。。。？？

    const counter: LocationStateDefinition<number> = {
      name: "count",
      defaultValue: 0,
      storeName: "session",
    };

これを書き換えてエラーになって安心してました。
hooks経由だと確かにダメですね。。。

AkifumiSato · 2023-09-12T15:11:13Z

デフォルト引数指定足したらうまく行きました。
e6b98a1

    // ✅
    const counter: LocationStateDefinition<number> = {
      name: "count",
      defaultValue: 0,
      storeName: "session",
    };
    // ✅
    const counter: LocationStateDefinition<number, "other"> = {
      name: "count",
      defaultValue: 0,
      storeName: "other",
    };
    // ❌
    const counter: LocationStateDefinition<number> = {
      name: "count",
      defaultValue: 0,
      storeName: "other",
    };
    // ❌
    const counter: LocationStateDefinition<number, "other"> = {
      name: "count",
      defaultValue: 0,
      storeName: "session",
    };

    // ✅
    const [count, setCount] = useLocationState({
      name: "count",
      defaultValue: 0,
      storeName: "session",
    });
    // ✅
    const [count, setCount] = useLocationState<number, 'other'>({
      name: "count",
      defaultValue: 0,
      storeName: "other",
    });
    // ❌
    const [count, setCount] = useLocationState({
      name: "count",
      defaultValue: 0,
      storeName: "other",
    });
    // ❌ 
   const [count, setCount] = useLocationState<number, 'other'>({
      name: "count",
      defaultValue: 0,
      storeName: "session",
    });

koichik · 2023-09-12T15:22:21Z

GJ!!!
しかしストア名をカスタマイズするとステートの型指定が必須？それはイマイチ。。。
~~型引数の並びを逆にする？~~ それはできないか

AkifumiSato · 2023-09-12T15:29:33Z

そうしたかったんですが逆にすると↓のエラーになってしまって、他にやりよう思いつかずな状況です...

error TS2706: Required type parameters may not follow optional type parameters.

koichik · 2023-09-12T16:43:42Z

ちょっと発想を変えてみる

export type LocationStateDefinition<StoreNames extends string, T> = {...};

const _useLocationState = <StoreNames extends string, T>(
  definition: LocationStateDefinition<StoreNames, T>,
): [T, SetState<T>] => {
  const storeState = _useLocationStateValue(definition);
  const setStoreState = _useLocationSetState(definition);
  return [storeState, setStoreState];
};

const _useLocationStateValue = <StoreNames extends string, T>(
  definition: LocationStateDefinition<StoreNames, T>,
): T => {
  ...
};

const _useLocationSetState = <StoreNames extends string, T>(
  definition: LocationStateDefinition<StoreNames, T>,
): SetState<T> => {
  ...
};

export const withStoreNames = <StoreNames extends string>() =>
  ({
    useLocationState: _useLocationState,
    useLocationStateValue: _useLocationStateValue,
    useLocationSetState: _useLocationSetState,
  } as {
    useLocationState: <T>(
      definition: LocationStateDefinition<StoreNames, T>,
    ) => [T, SetState<T>];
    useLocationStateValue: <T>(
      definition: LocationStateDefinition<StoreNames, T>,
    ) => T;
    useLocationSetState: <T>(
      definition: LocationStateDefinition<StoreNames, T>,
    ) => SetState<T>;
  });

export const { useLocationState, useLocationStateValue, useLocationSetState } =
  withStoreNames<DefaultStoreNames>();

独自のストア名を使う場合は

const { useLocationState } = withStoreNames<"foo" | "bar">()

export const Component = () => {
  const [count, setCount] = useLocationState({
    ...
    storeName: "foo",
  });
};

通常はプロジェクトの一カ所でexport const {...} = withStoreNames<...>()して他はそれをimportして使えばいいから手間に感じることはないと思う

AkifumiSato · 2023-09-13T00:40:19Z

なるほどです！
実は最初似たような実装試してて、Contextまで含めてStoreNameを指定できる形を目指してうまくいかず、hooksだけ対応すると型のために関数生やすことになるので反対されるかなと思って今の形にしてました。
すいません、相談すればよかったですね。。。
一旦↑の形で修正して、後続のカスタムStore周り実装するらへんでContextもStoreName指定できるようにしたいです。
（無理ですかね...）
あと↑の実装に関していくつか質問です。

_useLocationStateとなってますがexportしないなら命名はuseLocationStateのままでもいいですかね？
withStoreNamesですがcreateLocationHooksみたいな命名にしたくなったんですがどうでしょう？どっか一箇所でしか使わないしimportパスでわかるからLocationとか入ってなくてもいいですかね？
デフォルトのStore指定版として、useLocationStateはそのまま引き続きexportするのは微妙ですか？Storeをカスタムしない人は直接importして使えるならそっちの方がやりやすいかなと思い...

koichik · 2023-09-13T01:52:43Z

デフォルトのStore指定版として、useLocationStateはそのまま引き続きexport

してます、最後の↓がそれです

export const { useLocationState, useLocationStateValue, useLocationSetState } =
  withStoreNames<DefaultStoreNames>();

_useLocationStateとなってますがexportしないなら命名はuseLocationStateのまま

上に書いたデフォルトストア版と重複定義になっちゃうんですよね
デフォルトストア版の定義 (export) をhooks.tsとは別ファイルにすればアンスコ付ける必要はなくなります

withStoreNamesですがcreateLocationHooksみたいな命名

いいですよ
でもhooksを「作って」はいないんですよね、、、なのでgetHooks()とかの方がいいかもしんない
命名としては「何を返すか」と「なんのために使うのか」で前者に着目するとcreateHooks()またはgetHooks()とかになり、後者に着目したのがwithStoreNames()ですね
両方をミックスするとgetHooksWithStoreNames()とか
あるいは

type MyStoreNames = "foo" | "bar";
const {...} = getHooksWith<MyStoreNames>();

というのもアリかもしれない

ContextもStoreName指定できるようにしたい

これに対してはあまりポジティブではない、、、けどやってくれて構いません
なぜポジティブじゃないかというと自作自演な型にしかならない気がしてるからです
でもそうじゃない実装ができるなら全然アリなのでトライしてみてください！

koichik · 2023-09-13T09:32:06Z

上に貼ったコードのgenerics、<StoreNames, T>の並びになってるのは前のコメントでやろうとしてた名残で実際は必要ないと思うので元の<T, StoreNames>のままでも構わないです

AkifumiSato · 2023-09-13T11:38:43Z

getHooksWithで実装してみました！
ffc4b8a

なぜポジティブじゃないかというと自作自演な型にしかならない気がしてるからです
でもそうじゃない実装ができるなら全然アリなのでトライしてみてください！

なるほどです
どっかでチャレンジしてみるかもですが一旦このPRでは見送ろうかと思います🙇‍♂️

koichik

ちょっとコメントしてますが修正してもらったらマージOKです！乙でした！

koichik · 2023-09-13T12:28:18Z

packages/location-state-core/src/hooks.ts

  name: string;
  defaultValue: T;
-  storeName: StoreName | string;
+  // Avoid inference to rigorously check type arguments
+  storeName: StoreName extends infer S ? S : never;


この修正もう不要ですよね？
storeName: StoreNameでいいはず

koichik · 2023-09-13T12:30:10Z

packages/location-state-core/src/hooks.ts

@@ -25,16 +29,19 @@ const useStore = (storeName: StoreName | string) => {
  return store;
 };

-export const useLocationState = <T>(
-  definition: LocationStateDefinition<T>,
+const _useLocationState = <T, StoreName extends string = DefaultStoreNames>(


型引数のデフォルトも不要かなと

koichik · 2023-09-13T12:30:23Z

packages/location-state-core/src/hooks.ts

-  definition: LocationStateDefinition<T>,
+const _useLocationStateValue = <
+  T,
+  StoreName extends string = DefaultStoreNames,


koichik · 2023-09-13T12:30:38Z

packages/location-state-core/src/hooks.ts

@@ -56,8 +63,8 @@ export const useLocationStateValue = <T>(
  return storeState;
 };

-export const useLocationSetState = <T>(
-  definition: LocationStateDefinition<T>,
+const _useLocationSetState = <T, StoreName extends string = DefaultStoreNames>(


koichik · 2023-09-13T12:39:40Z

packages/location-state-core/src/hooks.ts

@@ -78,3 +85,23 @@ export const useLocationSetState = <T>(
  );
  return setStoreState;
 };
+
+export const getHooksWith = <StoreNames extends string>() =>


自分のせいだけどStoreNamesとStoreNameで一貫性がなくなってますね。。。
DefaultStoreNamesも含めてどちらかに揃えたい
元々はデフォルトのストア (2つあるから複数) の名前なので複数形がいいかなと思ってDefaultStoreNamesにしてもらったんだけど、このファイルの型引数の場合は単独のストア名の型だから単数形にするべきですよねぇ
そうするとDefaultStoreNamesも単数形にすべきか。。。
なんかそんな気がしてきたので単数形に揃えますか
手間を増やして申し訳ない

koichik · 2023-09-13T12:41:29Z

packages/location-state-core/src/types.ts

@@ -1,6 +1,6 @@
 /// <reference types="navigation-api-types" />

-export type StoreName = "session" | "url";
+export type DefaultStoreNames = "session" | "url";


ということでDefaultStoreNameで

# Conflicts: # packages/location-state-core/package.json # packages/location-state-next/package.json

AkifumiSato · 2023-09-13T13:06:00Z

↑も修正しました！
f5eb191
マージします

StoreName type check not depends on infer

691cb76

AkifumiSato requested a review from koichik September 12, 2023 12:43

koichik reviewed Sep 12, 2023

View reviewed changes

rename DefaultStoreName to DefaultStoreNames

ce4b2f7

fix hooks's StoreName default type

e6b98a1

refactor internal hooks type

5558c02

add getHooksWith<StoreNames>

ffc4b8a

koichik approved these changes Sep 13, 2023

View reviewed changes

refactor StoreNames to StoreName, delete redundant descriptions.

f5eb191

# Conflicts: # packages/location-state-core/package.json # packages/location-state-next/package.json

AkifumiSato merged commit 516381f into main Sep 13, 2023
1 check passed

AkifumiSato deleted the store-name-type branch September 13, 2023 13:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StoreName type check not depends on infer #10

StoreName type check not depends on infer #10

AkifumiSato commented Sep 12, 2023

koichik commented Sep 12, 2023

koichik left a comment

koichik Sep 12, 2023

AkifumiSato Sep 12, 2023

koichik Sep 12, 2023

AkifumiSato Sep 12, 2023

koichik Sep 13, 2023

AkifumiSato commented Sep 12, 2023

AkifumiSato commented Sep 12, 2023 •

edited

Loading

koichik commented Sep 12, 2023 •

edited

Loading

AkifumiSato commented Sep 12, 2023

koichik commented Sep 12, 2023

AkifumiSato commented Sep 13, 2023

koichik commented Sep 13, 2023

koichik commented Sep 13, 2023

AkifumiSato commented Sep 13, 2023 •

edited

Loading

koichik left a comment

koichik Sep 13, 2023

koichik Sep 13, 2023

koichik Sep 13, 2023

koichik Sep 13, 2023

koichik Sep 13, 2023

koichik Sep 13, 2023

AkifumiSato commented Sep 13, 2023

StoreName type check not depends on infer #10

StoreName type check not depends on infer #10

Conversation

AkifumiSato commented Sep 12, 2023

koichik commented Sep 12, 2023

koichik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkifumiSato commented Sep 12, 2023

AkifumiSato commented Sep 12, 2023 • edited Loading

koichik commented Sep 12, 2023 • edited Loading

AkifumiSato commented Sep 12, 2023

koichik commented Sep 12, 2023

AkifumiSato commented Sep 13, 2023

koichik commented Sep 13, 2023

koichik commented Sep 13, 2023

AkifumiSato commented Sep 13, 2023 • edited Loading

koichik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkifumiSato commented Sep 13, 2023

AkifumiSato commented Sep 12, 2023 •

edited

Loading

koichik commented Sep 12, 2023 •

edited

Loading

AkifumiSato commented Sep 13, 2023 •

edited

Loading