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

[Proposal] useGlobalize generic type #54

Open
fend25 opened this issue Mar 18, 2020 · 7 comments
Open

[Proposal] useGlobalize generic type #54

fend25 opened this issue Mar 18, 2020 · 7 comments

Comments

@fend25
Copy link

fend25 commented Mar 18, 2020

Hello!

Didn't you think about some way to specify possible list of IDs?
For example:

// it is possible to build programmatically from your translations object, even nested
type AllowedIds = 'guests' | 'yes' | 'no' 

const formatMessage = useGlobalize<AllowedIds>()

formatMessage('yes') // ok
formatMessage('maybe') // compile time error, not runtime

possible generic could be like that (orig code):

export interface Globalize<IDType extends string =string> extends GlobalizeConfig, GlobalizeHelpers, Formatters {
  // ...
  formatMessage(
    id: IDType | IDType[],
    values?: string[] | Record<string, string>,
    options?: MessageFormatterOptions,
  ): string;
  // second overload same way
  // ...

It won't break code for users who don't use this generic (because the fallback is the old type: string).

It won't be useful for users who slash-combined ids: 'party/guests'. But I think than benefit gain worth it, because it makes the lib really type strong (strings are the weak place almost always).
So, for users who prefer to use complex paths (via slash), nothing is changed, meanwhile for users who don't want to rely on strings it would be a huge benefit.

@joshswan
Copy link
Owner

Definitely open to improving the experience around message identifiers. I'd prefer something a little more "automatic" though - perhaps something that works with loadMessages.

@fend25
Copy link
Author

fend25 commented Mar 19, 2020

Yeah, I was thinking about some way to make automatic check, but now there is one circumstanse: nested path (i.e. 'foo/bar').
Unfortunately, in the current Typescript version can not infer the type of string concatenation.
Example:

const a: 'a' = 'a'
const b: 'b' = 'b'
let c = a + b // type is string, not 'ab'
c = '123' // ok, no any errors

I've found an article about it https://medium.com/@flut1/deep-flatten-typescript-types-with-finite-recursion-cb79233d93ca , but there is no way to get string type.

I do automatic id type inferring via such trick:

const one = {/* translations */}
const two = {/* translations */}

const dictionary = {
  ...one,
  ...two,
}

const IDictionaryId = keyof typeof dictionary 

So, I can check whether the string is valid id at compile time. And for the complex IDs there should be some string concatenation, but I am really not sure whether there is a TS magic way to do it. I think now TS can not infer this, meanwhile building an object of all keys in some recursive function will cause string concatenation, which brake TS' inferring of object to it's interface.

Also there is a one more problem: messages object now is object of dictionaries per locale. It's assumed that all per-locale dictionaries should have the same shape, but in TS we can not rely on assuming, we have to definetely prove that they are the same. So, we have to do some checking of dictionariy sub-objects relying on one of the sub-objects. I am not sure that TS allows such trick and that the TS should do it, but it's just my thoughts.

I am not against of automatic ID validation, I am rooting for it at all. I am just trying to discuss some problems that we have and hear your thoughts about it.

Thank you anyway for your great work and your attention.

@joshswan
Copy link
Owner

Yeah I think we'll just have to ignore typing the nested/string syntax as TS doesn't support it. However, the array syntax for nested keys (e.g. ['foo', 'bar']) could be strongly typed:

export type MessageData = Record<string, Record<string, string | Messages>>;

export interface Globalize<
  M extends MessageData = { [id: string]: Record<string, string | Messages> }
> extends GlobalizeConfig, GlobalizeHelpers, Formatters {
  formatMessage<
    Locale extends keyof M,
    ID1 extends keyof M[Locale]
  >(
    id: [ID1],
    values?: string[] | Record<string, string | number>,
    options?: MessageFormatterOptions,
  ): string;
  formatMessage<
    Locale extends keyof M,
    ID1 extends keyof M[Locale],
    ID2 extends keyof M[Locale][ID1]
  >(
    id: [ID1, ID2],
    values?: string[] | Record<string, string | number>,
    options?: MessageFormatterOptions,
  ): string;
  // support for maybe 5 levels?
}

It's definitely not pretty, but using Globalize<typeof myMessagesObj> and the array syntax seems to work quite well this way. However, the following would still have to be considered/improved:

  • How should it behave with differently-shaped locales? The solution above only permits keys that are common to all.
  • How many levels of nesting to actually support?
  • How should single first-level keys be handled (i.e. should writing ['key'] be necessary)?
  • Assuming we go down the path of strongly typing the array syntax, should that be the preferred syntax in the docs?

Anyway, definitely room for improvement, but neither the article you linked nor various other libraries gave me any inspiration for something more "magical". What are your thoughts? Should one of us open a PR so we start playing around with solutions?

@fend25
Copy link
Author

fend25 commented Mar 19, 2020

Oh, you did a deep investigation!

So, you pose an eternal question - simplicity of API vs it's accuracy and complexity.

Basing on my experience, almost all TS typings that was trying to cover universal cases became too complex and not neat.

I don't believe there is a chance to resolve all these issues (at least the 4 you have posed) without overcomplication the API.

My opinion: for only this case, for such deep and hacky TS magic - don't chase the universal solution. Just provide some robust and clear-how-to-use solution. There is too much options and cases and users can pick their way themselves. Still I think we shouldn't complicate API for the less percent of users at the cost of API complication for the most part.
I prefer to follow the unix way - your lib does the translations, not the TS inferring serivce. And let it do the translations, the only one thing clear and robust.

You make this lib for fun. And you do this really well. I am rooting for the simplest and the cheapest way to reach the enough quality.

I am just adviser, the decision is yours :)

PS. sorry for language mistakes and for verbosity. Just don't know how to express simpler.

joshswan added a commit that referenced this issue Mar 20, 2020
This makes the Globalize interface and useGlobalize hook generic to allow typing of message
identifiers. By default, any string is valid (same behavior as before), but a specific list of
allowed identifiers can be used (e.g. using keyof typeof messages.locale).

re #54
@joshswan
Copy link
Owner

Just released your original proposal in v4.2.0. It's definitely the simplest and most flexible solution on the table.

We use this library internally at my company now (which was the main motivation behind the TypeScript rewrite), so I'll let people play around with this solution and see if more improvements (or complications) are warranted. Leaving this issue open in the meantime.

Thanks for your input on this one! And definitely let me know if you see room for improvement :)

@fend25
Copy link
Author

fend25 commented Mar 20, 2020

Thank you!

Definetely I will let you know if I'll have any ideas.

Now I am afraid that my idea cause API break: it's not possible to set type of useGlobalize globally, so the way is to provide some factory:

// globalizeFactory.ts
const {useGlobalize} = loadMessages(SomeMessageObject)
export useGlobalize

// someComponent.tsx
import {useGlobalize} from 'src/globalizeFactory.ts' //not from the lib

joshswan added a commit that referenced this issue Mar 20, 2020
This makes the Globalize interface and useGlobalize hook generic to allow typing of message
identifiers. By default, any string is valid (same behavior as before), but a specific list of
allowed identifiers can be used (e.g. using keyof typeof messages.locale).

re #54
@joshswan
Copy link
Owner

I wouldn't call it a "break" since the original string typing still applied. However, to avoid people getting their types messed up, I just released v4.2.1 which reverted the changes. And I re-released the current solution as v4.3.0-alpha.0.

The feedback I got so far was:

  • this solution works okay in specific areas of the application (e.g. a feature folder with related components and messages so importing the messages type and using it whenever using useGlobalize isn't too painful - though some question whether the benefit is worth it)
  • as you mentioned, it doesn't work globally, and continuously importing a global messages object type is annoying

The global issue could be resolved by improving the context provider, but ideally I really want something that doesn't get in the way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants