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

Add a new option enableWarning #44

Closed
wants to merge 2 commits into from

Conversation

zry656565
Copy link

  • Add a new option enableWarning to determine if showing warning messages.
  • It's helpful to disable warnings in prod environment.

@CLAassistant
Copy link

CLAassistant commented Jan 13, 2018

CLA assistant check
All committers have signed the CLA.

…ages

fix: add types for `enableWarning`

Add missing semicolon
@spherop
Copy link

spherop commented Feb 14, 2018

This is a must have for serious production use in my book. Is there a chance of merging?

@alexeyMohnatkin
Copy link
Contributor

It's better to pass callback function instead of flag

intl.init({ 
  locales, 
  currentLocale,
  notDefinedHandler: R.identity,
});

#47

@cwtuan
Copy link
Collaborator

cwtuan commented Mar 1, 2018

I prefer the solution from @alexeyMohnatkin. Please see the comment in the PR. Thanks.

@zry656565
Copy link
Author

@cwtuan So why not just accpet Alexey's solution? It's fine to me. What I want is to solve the problem, no matter how.

Btw, I think the variable name, notDefinedHandler, may not be proper for this line (https://github.com/alibaba/react-intl-universal/pull/47/files#diff-1fdf421c05c1140f6d71444ea2b27638R83)

@cwtuan
Copy link
Collaborator

cwtuan commented Mar 1, 2018

@zry656565 any suggestion for the variable name?

@zry656565
Copy link
Author

@cwtuan Maybe a more general name like warningLogger or warningHandler? idk, it's up to you.

@zry656565
Copy link
Author

And it would be better that the handler's type is defined as boolean | (message: string) => void in my opinion. because it's odd to define an empty function to disable warning. just a suggestion

@alexeyMohnatkin
Copy link
Contributor

I agree that notDefinedHandler is not the best name. What if I add callback for each warning to handle them separately?

{
  ...
  msgNotFound: console.warn,
  msgFormatFailed: console.warn,
  langNotSupported: console.warn,
}

@alexeyMohnatkin
Copy link
Contributor

alexeyMohnatkin commented Mar 3, 2018

@zry656565 I think it's bad practice to disable these warnings. You may never know that something's wrong

@cwtuan
Copy link
Collaborator

cwtuan commented May 13, 2018

@cwtuan cwtuan closed this May 13, 2018
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.

5 participants