Strongly type Message and use in main signature #16

seanf · 2018-01-16T14:09:13Z

No description provided.

BANG88 · 2018-01-16T14:24:21Z

Too many modifications. Its difficulty to understand what do you want from this PR or solve some problems?

baktun14 · 2018-01-16T15:07:04Z

Where are the descriptions going in the exported json file?

baktun14 · 2018-01-16T19:11:11Z

I have a question for you @seanf , as you seem to have a better understanding of the typescript reflection api.

I currently have a small issue with formatMessage, here's the example of what actually works:

const message = formatMessage({id: "test", defaultMessage: "test message"});

and what does not work:

let message;
if (condition) {
  message = formatMessage({id: "test_1", defaultMessage: "test message 1"});
} else {
  message = formatMessage({id: "test_2", defaultMessage: "test message 2"});
}

How do you debug this library's code? Do you simply use console.log to know what is what or do you have another technique?

Thank you in advance!

I'm still unsure if it would be a good idea to parse every formatMessage function calls because sometimes I use it with non static ids like this;

formatMessage({id: object.id, defaultMessage: object.defaultMessage})

Anyways let me know what you guys think!

seanf · 2018-01-16T23:10:56Z

You're right. I'll split it into two PRs. One for the changes to Message, one for the strict null check refactoring.

…

On Jan 17, 2018 00:24, "Qibang" ***@***.***> wrote: Too many modifications. Its difficulty to understand what do you want from this PR or solve some problems? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#16 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADTiw-xgB0HlzdHKR8ERA8v5pqOfFLfks5tLLEVgaJpZM4Rf1DR> .

seanf · 2018-01-16T23:20:58Z

I actually put in a TODO about that. The call to defineMessages and formatMessage are currently only picked up if they are used in a variable declaration. @BANG88, is there a reason why defineMessages does that? I'm thinking we could just pick up any function call to either method. Even without a var declaration. Maxime, as a workaround, you could use defineMessages first, then format one of the messages in the if statement. I'm not sure if babel-react-intl picks up calls to formatMessage without defineMessages.

…

On Jan 17, 2018 05:11, "Maxime Beauchamp" ***@***.***> wrote: I have a question for you @seanf <https://github.com/seanf> , as you seem to have a better understanding of the typescript reflection api. I currently have a small issue with formatMessage, here's the example of what actually works: const message = formatMessage({id: "test", defaultMessage: "test message"}); and what does not work: let message; if (condition) { message = formatMessage({id: "test_1", defaultMessage: "test message 1"}); } else { message = formatMessage({id: "test_2", defaultMessage: "test message 2"}); } How do you debug this library's code? Do you simply use console.log to know what is what or do you have another technique? Thank you in advance! I'm still unsure if it would be a good idea to parse every formatMessage function calls because sometimes I use it with non static ids like this; formatMessage({id: object.id, defaultMessage: object.defaultMessage}) Anyways let me know what you guys think! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADTiyr8pLoXY5KGRWpf1e5RbLZqQm1cks5tLPRPgaJpZM4Rf1DR> .

baktun14 · 2018-01-16T23:41:46Z

@seanf Ah I didn't see the TODO. Yea I'm using defineMessages for now, and it wouldn't be such a bad thing to only include the ones assigned to a variable, like it is right now.

seanf · 2018-01-16T23:54:37Z

@mbeauchamp7

How do you debug this library's code? Do you simply use console.log to know what is what or do you have another technique?

Mostly https://github.com/dsherret/ts-ast-viewer

I also found this article useful: http://blog.scottlogic.com/2017/05/02/typescript-compiler-api-revisited.html

seanf · 2018-01-17T00:18:02Z

@seanf Ah I didn't see the TODO. Yea I'm using defineMessages for now, and it wouldn't be such a bad thing to only include the ones assigned to a variable, like it is right now.

We only pick up function calls which pass in an object declaration, where the keys and values are statically known (identifiers and literals). I think babel-plugin-react-intl goes one better and can pick up things like concatenated string constants&literals: https://github.com/yahoo/babel-plugin-react-intl/blob/6f4a533921c8632f53218c00cc3be3042814eed8/src/index.js#L31-L40

Either way, this would skip dynamic examples like this:

{id: object.id, defaultMessage: object.defaultMessage}

@BANG88 @mbeauchamp7
I think it might be better to follow the lead of babel-plugin-react-intl (making it easier to switch between them) and thus:

ignore formatMessage - I don't think it's intended to define messages as a side effect, just format them. But we could make this an option.
not require a variable declaration.
It might help performance/accuracy to skip files which don't import react-intl too. This module name should be configurable.

It may not be easy to replicate another one of babel-plugin-react-intl's features - I think it might be able to handle renamed imports, eg

import {defineMessages as defMsgs} from 'react-intl';

So it might be good to list that as a known limitation (along with the concatenated constants thing, which I already ran into).

BANG88 · 2018-01-17T01:20:59Z

@seanf @mbeauchamp7 This library may not covered every use case. at first my point is make it work so PR are always welcome.

If we want export the message interface I think should be something like this:

export interface Message {
  id: string
  defaultMessage: string 
  description?: string 
}

baktun14 · 2018-01-17T02:52:44Z

@seanf @BANG88 To be honest, I'm fine with how the library is actually behaving. Ignoring files that don't import react-intl would definitely help performance, but I have a pretty big project and it's actually pretty fast.

I would like to keep the parsing of formatMessage as is, since it's pretty much the same as <FormatMessage /> and yea not require a variable **declaration.** would be cool but not necessary. I rarely or never rename imports, so up to you!

seanf · 2018-01-17T03:36:02Z

I think that's what I've used. Is it not? (Although I put id last, since tslint likes keys to be in order when you actually make one of these objects.) But as I said, I will separate that stuff from all the null check stuff. It should be clearer then.

…

On 17 January 2018 at 11:21, Qibang ***@***.***> wrote: @seanf <https://github.com/seanf> @mbeauchamp7 <https://github.com/mbeauchamp7> This library may not covered every use case. at first my point is make it work so PR are always welcome. If we want export the message interface I think should be something like this: export interface Message { id: string defaultMessage: string description?: string } — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADTi3X_ziT5xyzEn6CJM7FP86_3udKvks5tLUr7gaJpZM4Rf1DR> .

seanf · 2018-01-17T11:56:55Z

Assuming #17 goes in, I'll rebase this on top, then the diff should just show Message-related changes.

seanf · 2018-01-17T12:59:41Z

I have another commit which cleans up the JSX types (no more any!), but I'll put it in another PR later.

seanf@e462d9d -> seanf@b03e14b

…rface

BANG88 · 2018-01-19T07:39:51Z

index.ts

-interface Message {
-  [key: string]: string;
+// a Message with nullable fields, still under construction
+interface PartialMessage {


This could be done via Partial

BANG88 · 2018-01-19T07:40:08Z

index.ts

-// just a map of string to string
-interface Message {
-  [key: string]: string;
+// a Message with nullable fields, still under construction


This could be done via Partial<Message>

Yes, I just discovered that, thanks. I need to make things a bit more idiomatic.

BANG88 · 2018-01-19T07:42:49Z

index.ts

+}
+
+// are the required keys of a valid Message present?
+function isValidMessage(obj: PartialMessage): obj is Message {


difficulty to understand why need obj is Message can be inferred by TS

I think it's because the copying happens dynamically. There's no static way of knowing whether those property assignments will occur.

No. from this method your result must be a boolean type. am I right ?

This is another of those user-defined type guards.

BANG88 · 2018-01-19T07:44:38Z

index.ts

  return jsxMessages.concat(dm).concat(fm);
 }

+function notNull<T>(value: T | null): value is T {


Does this return a boolean value. if so why need value is T

It does return a boolean at runtime, but this bit of typing enables the result of filter to be Message instead of Message | null as far as the type checker is concerned. It's a user-defined type guard: https://www.typescriptlang.org/docs/handbook/advanced-types.html

@seanf Thanks. I didn't use this type guard before. 😄

BANG88 · 2018-01-19T08:17:33Z

could you rebase it so we can merge this PR.

seanf · 2018-01-19T08:30:15Z

could you rebase it so we can merge this PR.

When you say rebase, do you mean I should rebase the 7 commits on top of master? They're kind of a mess, so I would advise "squash and merge" instead. But if you want to rebase then merge, GitHub usually lets you do that (hit the green drop-down and select "rebase and merge") - it leaves the original commits as they are in the PR for historical purposes.

Or did you want me to squash it into one commit, and force push to this branch before you merge? I'm happy to do that if that's what you mean, but again, GitHub can do that automatically if you "squash and merge" (again it leaves the original commits in the PR). (Whereas if I force push a squashed merge, the history of this code review pretty much goes away.)

Either way, let me know what you want. Unless you just want to "squash and merge", in which case go ahead!

BANG88 · 2018-01-19T08:33:02Z

Or did you want me to squash it into one commit, and force push to this branch before you merge? I'm happy to do that if that's what you mean,
Yep. I will use Github's button for merge. sometimes I prefer review one commit so.

seanf added 3 commits January 16, 2018 15:35

Improve Message interface

3471992

Export Message interface; check nulls

04440f8

Remove arbitrary keys from Message interface

2b741bb

seanf requested a review from BANG88 January 16, 2018 14:09

seanf changed the title ~~Export Message interface; check nulls~~ wip: Export Message interface; check nulls Jan 17, 2018

seanf mentioned this pull request Jan 17, 2018

Enable strict null checks #17

Merged

Avoid copying Messages around

b2e7645

seanf force-pushed the better-message-interface branch from e462d9d to b2e7645 Compare January 17, 2018 12:56

seanf added 2 commits January 19, 2018 17:25

Merge remote-tracking branch 'origin/master' into better-message-inte…

6200a58

…rface

Don't export PartialMessage

f1a5ede

seanf changed the title ~~wip: Export Message interface; check nulls~~ Strongly type Message and use in main signature Jan 19, 2018

BANG88 reviewed Jan 19, 2018

View reviewed changes

Use Partial<Message>

7b73f4b

BANG88 approved these changes Jan 19, 2018

View reviewed changes

BANG88 merged commit a3ee268 into BANG88:master Jan 19, 2018

seanf deleted the better-message-interface branch January 19, 2018 08:36

Strongly type Message and use in main signature #16

Strongly type Message and use in main signature #16

Uh oh!

Conversation

seanf commented Jan 16, 2018

Uh oh!

BANG88 commented Jan 16, 2018

Uh oh!

baktun14 commented Jan 16, 2018

Uh oh!

baktun14 commented Jan 16, 2018

Uh oh!

seanf commented Jan 16, 2018 via email

Uh oh!

seanf commented Jan 16, 2018 via email

Uh oh!

baktun14 commented Jan 16, 2018

Uh oh!

seanf commented Jan 16, 2018

Uh oh!

seanf commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BANG88 commented Jan 17, 2018

Uh oh!

baktun14 commented Jan 17, 2018

Uh oh!

seanf commented Jan 17, 2018 via email

Uh oh!

seanf commented Jan 17, 2018

Uh oh!

seanf commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BANG88 Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BANG88 commented Jan 19, 2018

Uh oh!

seanf commented Jan 19, 2018

Uh oh!

BANG88 commented Jan 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

seanf commented Jan 17, 2018 •

edited

Loading

seanf commented Jan 17, 2018 •

edited

Loading

BANG88 Jan 19, 2018 •

edited

Loading