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(@formatjs/ts-transformer): allow to override user defined message id via overrideIdFn as function #1926

Merged
merged 1 commit into from Aug 7, 2020

Conversation

nodkz
Copy link
Contributor

@nodkz nodkz commented Aug 5, 2020

This PR introduces the following logic:

  1. overrideIdFn is a string
  • if msg.id is defined leave it unchanged
  • if msg.id is empty then use interpolateName with provided template
  1. overrideIdFn is a function
  • pass msg.id to function as first arg. Allow to user decide what to do with id - it may be empty or contain already defined value

… id via `overrideIdFn` as function

This commit introduces the following logic:
1) overrideIdFn is a string
  - if msg.id is defined leave it unchanged
  - if msg.id is empty then use interpolateName with provided template
2) overrideIdFn is a function
  - pass msg.id to function as first arg. Allow to user decide what to do with id - it may be empty or contain already defined value
@nodkz
Copy link
Contributor Author

nodkz commented Aug 5, 2020

I cannot start bazel on my macbook:

Screen Shot 2020-08-05 at 20 49 12

So I launch Jest manually and update existing snapshots without bazel.

@nodkz
Copy link
Contributor Author

nodkz commented Aug 5, 2020

I spent several hours to bazel without luck. @longlho please run tests on your machine before merge. Of course, if you accept these changes.

@longlho
Copy link
Member

longlho commented Aug 5, 2020

looks like you forgot to initialize git submodule per https://github.com/formatjs/formatjs#development

@longlho
Copy link
Member

longlho commented Aug 5, 2020

looks like this change is not backwards compatible. Can you explain the use case for #2 above? Why does overrideIdFn is a function need to behave differently from when it's a pattern?

@nodkz
Copy link
Contributor Author

nodkz commented Aug 5, 2020

I expect that ts-transformer's and babel-plugin-react-intl's function overrideIdFn works in the same manner. BUT currently it's not.

With babel, I can override msg.id provided by user. With ts transformer I cannot do this. Take a look at the following code – overrideIdFn is called only when msg.id is empty:

if (!msg.id && msg.defaultMessage && overrideIdFn) {

But with babel you can override it even if msg.id already defined by user –

if (overrideIdFn) {
id = overrideIdFn(id, defaultMessage, description, filename);
} else if (!id && idInterpolationPattern && defaultMessage) {
id = interpolateName(
{sourcePath: filename} as any,
idInterpolationPattern,
{
content: description
? `${defaultMessage}#${description}`
: defaultMessage,
}
);
}


Anyway, it's quite strange that overrideIdFn has the following typedef in TypeScript where first arg id always is empty (because this function is never called if msg.id is present):

export type InterpolateNameFn = (
id?: string,
defaultMessage?: string,
description?: string,
filePath?: string
) => string;

So, in other words, I feel that the behavior for overrideIdFn should be

  • it's template string then use it only if msg.id is empty.
  • if it's a function then use it for every message (and no matter exists msg.id or not). User can realize any logic what he needs, like in my case: if msg.id exists I want to ensure that exists prefix for msg.id and if it does not then add it. Like I did in my app with babel plugin. But for a package where is used typescript compilation - I cannot reuse the same logic. I'm not able to modify existed id.

PS. About bazel. My bad and you're right - I didn't init submodules.

@longlho longlho changed the title fix(@formatjs/ts-transformer): allow to override user defined message id via overrideIdFn as function feat(@formatjs/ts-transformer): allow to override user defined message id via overrideIdFn as function Aug 7, 2020
@longlho longlho merged commit 6af7846 into formatjs:main Aug 7, 2020
@longlho
Copy link
Member

longlho commented Aug 7, 2020

Thanks a lot for your contributions!

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.

None yet

2 participants