Introduce generic `warnOnce` function for warning messages #22109

empyrical · 2018-11-04T06:32:15Z

Summary

This pull request adds a function called warnOnce that prints a warning message to the console once per session.

It uses a unique key per callsite to help ensure that the message is not printed multiple times.

Changelog

[General] [Added] - Added new warnOnce function for printing a message to the developer console once per session
[General] [Changed] - Changed the warnings in react-native-implementation to use warnOnce

Test Plan

I have tested this in RNTester, and the warning appears as expected. Flow and ESLint have also been ran.

elicwhite · 2018-11-04T17:38:39Z

I really want to land this but Facebook hasn’t yet swapped over to the community one and we need to build a tad bit of infrastructure to support pulling in those 3rd party modules.

Can we delay adding this warning for a few weeks until we can make that switch?

elicwhite · 2018-11-04T17:40:56Z

Libraries/Utilities/warnMoved.js

+ * @param {string} [renamed] - Optional; name of the API in the dest module,
+ *                             if it was renamed
+ */
+function warnMoved(api: string, destModule: string, renamed?: string) {


If you make another function called warnOnce then it can be used for the existing warnings in the api as well.

sounds like a good idea! with the delay, maybe I should instead split this helper function into its own pull request?

Split maybe a good idea. We may have more usage for this if we move more component out.

empyrical · 2018-11-04T17:45:44Z

That sounds good!

I wonder, maybe for cases like this we could have an __FB__ or __OSS__ variable, so stuff could be stripped out of bundles like they can with if(__DEV__) blocks

elicwhite · 2018-11-04T17:51:20Z

That seems reasonable but I would worry that it would make it too easy for FB to have a diverging version of react-native which would be detrimental for everyone. For now, I'd rather keep the pressure on us to fix our stuff and get out of the way of progress. :) We currently have the work to migrate to the community WebView on our short term roadmap

empyrical · 2018-11-04T18:21:04Z

For the JS side of things, perhaps we should try and make a codemod to help rewrite imports of {WebView}. I imagine that's only a small part of the work that needs to be done, however!

elicwhite · 2018-11-04T18:55:14Z

Writing a codemod would be great! The community will need that when they upgrade to the version without WebView. If you don't want to work on it I'm sure there is someone around who would appreciate that opportunity!

cpojer · 2018-12-07T05:54:19Z

What's the status of this PR?

empyrical · 2018-12-07T12:41:56Z

Looks like I have to resolve a merge conflict, but I think the issue @TheSavior brought up above might still be relevant (I also need to still split this up into a separate helper function too)

cpojer · 2018-12-10T01:47:28Z

Libraries/Utilities/warnMoved.js

+
+  warning(
+    false,
+    `'${api}' has moved to another module and will be removed from 'react-native' ` +


Could we add something like:

yarn add ${destModule}

to the instructions?

hramos · 2019-01-15T22:17:38Z

I missed this PR way back then. #22980 landed, which adds the deprecation warning. We should still consider #22109 as a way to handle exported components in a consistent manner.

empyrical · 2019-01-16T02:57:41Z

I've been meaning to address this PR's issues, I will try to get to it this week!

cpojer · 2019-02-04T15:41:44Z

@empyrical do you wanna pick this back up or close it?

empyrical · 2019-02-04T20:01:40Z

It looks like I took so long to pick this back up that there is another warning in place on master now. Sorry about that! 😅

I can pick this back up though, it would still be useful for the generic "warnOnce()" function so all the warnings in react-native-implementation.js don't need to add x_warning_showed variables, hopefully cleaning things up a bit.

cpojer · 2019-02-04T20:23:33Z

Yes exactly! A generic warning function is still valuable. Do you wanna modify your existing PR to do that or close this one and open a new one?

empyrical · 2019-02-04T21:01:56Z

I've modified this PR instead of making a new one 👍

facebook-github-bot

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

cpojer

This is nice, thank you :)

react-native-bot · 2019-02-05T10:30:06Z

@empyrical merged commit 9a7fff9 into facebook:master.

…22109) Summary: This pull request adds a function called `warnOnce` that prints a warning message to the console once per session. It uses a unique key per callsite to help ensure that the message is not printed multiple times. [General] [Added] - Added new `warnOnce` function for printing a message to the developer console once per session [General] [Changed] - Changed the warnings in `react-native-implementation` to use `warnOnce` Pull Request resolved: facebook#22109 Differential Revision: D13955887 Pulled By: cpojer fbshipit-source-id: aa51ac427a80cc0554a6bcc915715821d0bd5439

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 4, 2018

react-native-bot added 🔶Components Component: View Component: WebView Related to the WebView component. labels Nov 4, 2018

elicwhite reviewed Nov 4, 2018

View reviewed changes

cpojer reviewed Dec 10, 2018

View reviewed changes

hramos removed the 🔶Components label Jan 24, 2019

Warn that 'WebView' has moved to 'react-native-webview'

c212a1f

Make warnMoved a more generic function

39fe07c

empyrical force-pushed the warn-webview branch from 35495d3 to 39fe07c Compare February 4, 2019 20:46

empyrical changed the title ~~Warn that 'WebView' has moved to 'react-native-webview'~~ Introduce generic warnOnce function for warning messages Feb 4, 2019

facebook deleted a comment from pull-bot Feb 5, 2019

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 5, 2019

facebook-github-bot approved these changes Feb 5, 2019

View reviewed changes

cpojer approved these changes Feb 5, 2019

View reviewed changes

facebook-github-bot closed this in 9a7fff9 Feb 5, 2019

facebook locked as resolved and limited conversation to collaborators Feb 5, 2019

react-native-bot added the Merged This PR has been merged. label Feb 5, 2019

hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 6, 2019

facebook-github-bot added the Contributor A React Native contributor. label Apr 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce generic `warnOnce` function for warning messages #22109

Introduce generic `warnOnce` function for warning messages #22109

empyrical commented Nov 4, 2018 •

edited

Loading

elicwhite commented Nov 4, 2018

elicwhite Nov 4, 2018

empyrical Nov 4, 2018

gengjiawen Dec 7, 2018

empyrical commented Nov 4, 2018

elicwhite commented Nov 4, 2018

empyrical commented Nov 4, 2018

elicwhite commented Nov 4, 2018

cpojer commented Dec 7, 2018

empyrical commented Dec 7, 2018

cpojer Dec 10, 2018

hramos commented Jan 15, 2019

empyrical commented Jan 16, 2019

cpojer commented Feb 4, 2019

empyrical commented Feb 4, 2019

cpojer commented Feb 4, 2019

empyrical commented Feb 4, 2019

facebook-github-bot left a comment

cpojer left a comment

react-native-bot commented Feb 5, 2019

Introduce generic warnOnce function for warning messages #22109

Introduce generic warnOnce function for warning messages #22109

Conversation

empyrical commented Nov 4, 2018 • edited Loading

Summary

Changelog

Test Plan

elicwhite commented Nov 4, 2018

elicwhite Nov 4, 2018

Choose a reason for hiding this comment

empyrical Nov 4, 2018

Choose a reason for hiding this comment

gengjiawen Dec 7, 2018

Choose a reason for hiding this comment

empyrical commented Nov 4, 2018

elicwhite commented Nov 4, 2018

empyrical commented Nov 4, 2018

elicwhite commented Nov 4, 2018

cpojer commented Dec 7, 2018

empyrical commented Dec 7, 2018

cpojer Dec 10, 2018

Choose a reason for hiding this comment

hramos commented Jan 15, 2019

empyrical commented Jan 16, 2019

cpojer commented Feb 4, 2019

empyrical commented Feb 4, 2019

cpojer commented Feb 4, 2019

empyrical commented Feb 4, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

cpojer left a comment

Choose a reason for hiding this comment

react-native-bot commented Feb 5, 2019

Introduce generic `warnOnce` function for warning messages #22109

Introduce generic `warnOnce` function for warning messages #22109

empyrical commented Nov 4, 2018 •

edited

Loading