Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

The "Copy element props" feature does not do a deep-copy #843

Closed
aozdemir-coursera opened this issue Jul 12, 2017 · 7 comments
Closed

The "Copy element props" feature does not do a deep-copy #843

aozdemir-coursera opened this issue Jul 12, 2017 · 7 comments

Comments

@aozdemir-coursera
Copy link

The copy props feature was introduced in #837. It's awesome, but has a substantial flaw: it only copies as deeply as the user has expanded the properties in the property explorer.

Reproduction steps:

  1. Go to Coursera's home page. If you were logged in, log out.
  2. Open the React extension.
  3. Search the inspector for "FrontPage2016" and select the node.
  4. Copy its props and paste them into a temporary file: before.json.
  5. Expand the naptime prop in the prop explorer. Then unexpand it.
  6. Copy the same node's props and paste into a temporary file: after.json.
  7. diff before.json after.json
  8. Cry

I have no familiarity with the React DevTools codebase, but I suspect that this is a side-effect of a lazy system designed to minimize communication with the React runtime. Assuming there is a way to say "Recursively eagerly evaluate this piece of data", there should be an easy fix.

@jaredly
Copy link
Contributor

jaredly commented Jul 13, 2017

We'd probably want to add a new function to the backend, which would be "give me a json representation of the props" and just feed it straight up, instead of recursively expanding through the current API.

@bvaughn
Copy link
Contributor

bvaughn commented Jul 13, 2017

That would be a nice improvement to the feature 😄

@zinoviev
Copy link
Contributor

zinoviev commented Jul 18, 2017

I implement asking JSON directly from backend as @jaredly suggest, but got an error when passing a resulted string to a clipboard on bridge callback: Unable to copy. Perhaps it's not available in your browser?

According to https://github.com/lgarron/clipboard.js

Note: in most browsers, copying is only allowed if clipboard.copy() is triggered in 
direct response to a user gesture like a click or a key press.

Seems this should work in web extension environment https://developer.mozilla.org/ru/Add-ons/WebExtensions/Interact_with_the_clipboard but will not work in plain shell

I don't think I have time to work on this, so someone might use props passing back code for reference - zinoviev@9d60964

@zinoviev
Copy link
Contributor

zinoviev commented May 16, 2018

Well, I play with new clipboard API and found it's still impossible to write stuff without direct user interaction https://w3c.github.io/clipboard-apis/#h-clipboard-write-permission and when we communicate with backend we lost an opportunity to use the clipboard.

But I check my previous attempt inside chrome extensions shell and surprisingly it's work as it is another permission scheme in extension environment. So I think, I can finish and send PR. But where is an issue with plain shell, so we need to choose what copy - all props or just first level.

  • Is were an option to check at runtime what shell we are running? @jaredly @bvaughn
  • If not, we can try to perform a deep copy, if failed turn on fallback to the old way and show warning in console. Caveat - user lost his operation, because "direct interaction" window will be lost. But on the other hand, assuming plain shell user it's dev tools developers it's probably not a big problem.

@bvaughn
Copy link
Contributor

bvaughn commented May 16, 2018

Is were an option to check at runtime what shell we are running?

Where would we need to know about this? Frontend or backend?

Assuming this works for Chrome + Firefox, we could let the frontend UI know by adding a prop to the config object that we pass to the shared Panel. We could perhaps do a similar thing in the backend setup.

Disclaimer: I haven't thought this through at all. 😄

@zinoviev
Copy link
Contributor

@bvaughn I'm going to add a switch in frontend to check if we are in extension, and if yes perform a deep copy or use copy first level props overwise. So I guess config props will work. Thanks!

@bvaughn
Copy link
Contributor

bvaughn commented Aug 19, 2019

React DevTools has been rewritten and recently launched a new version 4 UI. The source code for this rewrite was done in a separate repository and now lives in the main React repo (github.com/facebook/react).

Because version 4 was a total rewrite, and all issues in this repository are related to the old version 3 of the extension, I am closing all issues in this repository. If you can still reproduce this issue, or believe this feature request is still relevant, please open a new issue in the React repo: https://github.com/facebook/react/issues/new?labels=Component:%20Developer%20Tools

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants