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

deps: Use @react-native-clipboard/clipboard #4502

Closed
wants to merge 1 commit into from

Conversation

rajprakash00
Copy link
Contributor

@rajprakash00 rajprakash00 commented Feb 28, 2021

Clipboard in React Native core is deprecated. See here.
Although no visible bug occurs but warning errors are
shown in console to use @react-native-community/clipboard
which was recently migrated to @react-native-clipboard/clipboard here.

So,migrated to it.

Steps to reproduce the bug:

  • Go to any message-view
  • try copying the message using Copy to Clipboard
  • See console for warning errors

Additional Context:
Since I don't have MacOS so haven't linked & tested it in iOS.

Signed-off-by: rajprakash00 rajprakash1999@gmail.com

@chrisbobbe
Copy link
Contributor

Interesting, thanks for finding this! I see that deprecation warning in RN's Clipboard module doc.

I actually haven't seen the errors you describe in the commit message. When and where are you seeing those? It makes me wonder if I'm missing an important source of information in my normal development setup. 🙂

@rajprakash00
Copy link
Contributor Author

Oh! My bad ,I should have written steps to reproduce the bug.
Anyways the warning errors appears in console whenever we try the copy to clipboard option to copy any message from message-view
@chrisbobbe Is it helpful now?

@chrisbobbe
Copy link
Contributor

whenever we try the copy to clipboard option to copy any message from message-view

OK, great, that's helpful! Let's include that in the commit message. 🙂

Copy link
Contributor

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just tested on iOS, and it works! See one question below (and a note above); otherwise, this looks good.

@@ -0,0 +1,63 @@
// flow-typed signature: 12bef3bad479f254c9e8da5a51023c58
Copy link
Contributor

@chrisbobbe chrisbobbe May 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the commit message:

Also,this library has built-in Flow support, so we don't need to make our
own libdef.

Is this true? I see that you've added a stub libdef with no types in it. If you delete that, do we get good type-checking (i.e., Flow correctly complains when you try to do something wrong)?

  • If so, then we should just delete the stub libdef.
  • If not, then we should write our own libdef; you can read some case studies in our libdefs doc. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chrisbobbe Thanks for review!

Hmm... I remember now , the problem occured was that this error Importing from an untyped module makes it any and is not safe! Did you mean to add // @flow to the top of @react-native-clipboard/clipboard?Flow(untyped-import) always occured
And after some research I found I have to add a stub libdef for this(which Ithink it should be optional here 🤔)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK; that sounds like a sign that "Also,this library has built-in Flow support" is wrong. 🙂 Is there still something that leads you to believe that the library has built-in Flow support?

As mentioned above, if it turns out that it doesn't come with types, we'll have to write our own with a meaningful libdef.

Copy link
Contributor Author

@rajprakash00 rajprakash00 May 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK , now I get it...seems earlier I misunderstood the built-in Flow support thing pretty much :( .

As mentioned above, if it turns out that it doesn't come with types, we'll have to write our own with a meaningful libdef.

I'm a little confused about this,
AFAIK @react-native-clipboard/clipboard has published libdef available(so we can get autogenerated libdef stub for it,which I've already done in this PR) ,
Or do we have to write separate libdef mentioning proper types , for e.g, we have for react-native-webview

Copy link
Contributor

@chrisbobbe chrisbobbe May 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, OK, so the thing we usually do to see if there's a libdef published by flow-typed is this:

flow-typed install @react-native-clipboard/clipboard

I've tried this command just now, and it gave this output:

[@octokit/rest] `const Octokit = require("@octokit/rest")` is deprecated. Use `const { Octokit } = require("@octokit/rest")` instead
• Searching for 1 libdefs...
• rebasing flow-typed cache...
!! No flow@v0.128.0-compatible libdefs found in flow-typed for the explicitly requested libdefs. !!

Consider using `flow-typed create-stub @react-native-clipboard/clipboard` to generate an empty libdef that you can fill in.

(I have no idea what the @octokit/rest thing is about; ignoring that.)

Notice the "No […] libdefs found in flow-typed […]." This is pretty normal, unfortunately.

The file you made looks like it was created by running the recommended command, flow-typed create-stub @react-native-clipboard/clipboard. But, as the rest of that sentence says (and as the generated file says), the file you get from that is basically empty; there's some boilerplate but nothing substantive. (In other words, it's a stub.) It won't have useful types until we write some. 🙂

The libdef for react-native-webview is an example where we wrote one, but I expect this one would be much easier than that—we use just one function it provides, Clipboard.setString, and that function's type seems pretty simple. Its parameters are described here, and I think it probably returns undefined (could check the implementation in node_modules/@react-native-clipboard/clipboard though if you wanted to be sure).

@rajprakash00
Copy link
Contributor Author

@chrisbobbe I've updated the changes, please take a look.

@chrisbobbe
Copy link
Contributor

Looks good, thanks! Please squash the second commit into the first; it's neatest to add a libdef in the same commit that we add the library itself. Also (and I'm not sure this is documented anywhere), our custom or at-all edited libdefs should go in flow-typed/, not flow-typed/npm. Also, when you squash the commits together, please make sure the commit message doesn't say that the library has built-in Flow types, since that's confusing (our custom libdef is where the types live). 🙂

Thanks!

`Clipboard` in React Native core is deprecated [1]. Because of that warning
errors are shown(whenever we try the `copy to clipboard` option to copy any
message from message-view) in console to use`@react-native-community/clipboard`
which was recently migrated to `@react-native-clipboard/clipboard` [2].
So, use it.

Also,write our own custom libdef for this with proper definitions by taking
notes from `@react-native-clipboard/clipboard/dist/Clipboard.d.ts`.

[1]: https://reactnative.dev/docs/clipboard
[2]: react-native-clipboard/clipboard#87

Signed-off-by: rajprakash00 <rajprakash1999@gmail.com>
@rajprakash00
Copy link
Contributor Author

@chrisbobbe all the changes done.

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Mar 10, 2022
`Clipboard` in RN core is deprecated; see warning at
  https://reactnative.dev/docs/clipboard

Reportedly [1], we've been getting console warnings that we should
use @react-native-community/clipboard instead of the RN-core module.
Do basically that, except we use its new name,
@react-native-clipboard/clipboard; see
  react-native-clipboard/clipboard#87

Put the types in a .js.flow file, rather than a libdef in
`flow-typed/`, inspired by Greg's commit 007dea3.

Supersedes: zulip#4502
Co-authored-by: rajprakash00 <rajprakash1999@gmail.com>
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Mar 29, 2022
`Clipboard` in RN core is deprecated; see warning at
  https://reactnative.dev/docs/clipboard

We've reportedly been getting console warnings that we should use
@react-native-community/clipboard instead of the RN-core module. Do
basically that, except we use its new name,
@react-native-clipboard/clipboard; see
  react-native-clipboard/clipboard#87

Put the types in a .js.flow file, rather than a libdef in
`flow-typed/`, inspired by Greg's commit 007dea3.

Supersedes: zulip#4502
Co-authored-by: rajprakash00 <rajprakash1999@gmail.com>
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Apr 5, 2022
`Clipboard` in RN core is deprecated; see warning at
  https://reactnative.dev/docs/clipboard

We've reportedly been getting console warnings that we should use
@react-native-community/clipboard instead of the RN-core module. Do
basically that, except we use its new name,
@react-native-clipboard/clipboard; see
  react-native-clipboard/clipboard#87

Put the types in a .js.flow file, rather than a libdef in
`flow-typed/`, inspired by Greg's commit 007dea3.

Supersedes: zulip#4502
Co-authored-by: rajprakash00 <rajprakash1999@gmail.com>
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Apr 5, 2022
`Clipboard` in RN core is deprecated; see warning at
  https://reactnative.dev/docs/clipboard

We've reportedly been getting console warnings that we should use
@react-native-community/clipboard instead of the RN-core module. Do
basically that, except we use its new name,
@react-native-clipboard/clipboard; see
  react-native-clipboard/clipboard#87

Put the types in a .js.flow file, rather than a libdef in
`flow-typed/`, inspired by Greg's commit 007dea3.

Supersedes: zulip#4502
Co-authored-by: rajprakash00 <rajprakash1999@gmail.com>
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Jan 26, 2023
`Clipboard` in RN core has been deprecated for a while, and in RN's
v0.71 doc they finally mark it "removed":
  https://reactnative.dev/docs/clipboard

We've reportedly (zulip#4502) been getting console warnings that we
should use @react-native-community/clipboard instead of the RN-core
module. Do basically that, except we use its new name,
@react-native-clipboard/clipboard; see
  react-native-clipboard/clipboard#87

Supersedes: zulip#4502
Co-authored-by: rajprakash00 <rajprakash1999@gmail.com>
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this pull request Jan 26, 2023
`Clipboard` in RN core has been deprecated for a while, and in RN's
v0.71 doc they finally mark it "removed":
  https://reactnative.dev/docs/clipboard

We've reportedly (zulip#4502) been getting console warnings that we
should use @react-native-community/clipboard instead of the RN-core
module. Do basically that, except we use its new name,
@react-native-clipboard/clipboard; see
  react-native-clipboard/clipboard#87

Supersedes: zulip#4502
Co-authored-by: rajprakash00 <rajprakash1999@gmail.com>
@gnprice
Copy link
Member

gnprice commented Jan 26, 2023

Updated by @chrisbobbe and merged as #5648. Thanks @rajprakash00 for the report and the fix!

@gnprice gnprice closed this Jan 26, 2023
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.

3 participants