-
-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
chore(debug-plugin): migrate to a new maintained JSON Viewer #9566
chore(debug-plugin): migrate to a new maintained JSON Viewer #9566
Conversation
Hi @mcrstudio! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
✅ [V2]Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
⚡️ Lighthouse report for the deploy preview of this PR
|
Thanks for your work on this I'm not super fan of the package you choose though, being maintained by a single person can lead to the same issue we are trying to fix right now 😅 Have you tried this one? It's more heavy (but not a big deal for this usecase imho) but is much more likely to stay maintained over time: https://github.com/reduxjs/redux-devtools/tree/main/packages/react-json-tree Also if possible I'd like to keep the amount of expanded items by default smaller. For example this page becomes less usable by default now: |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
@slorber I appreciate that, though I was choosing a library based on ease of implementation, as I'm very restricted for time. I have made an update that makes the new page behave almost identically to the old page. I think a large benefit of a tiny package like this is that it's only peer dependency is React, there are no difficult dependencies. If this goes unmaintained in the future, then at least it can be updated to a later version of React whilst another solution is found with less urgency? I'm not really able to make a commitment to switch out the library, as this was a quick fix I did over the weekend and I won't have much free time again for 2 weeks! |
I'm +1 to merging this as-is—I don't think it's worth time at this point to find a perfect solution and spend time verifying that it works, when it is causing actual issues in user land. |
@Josh-Cena @slorber I can also note that aesthetically, it's not as pleasing. We can, however, add some styles to modify the look and appearance but the indentation isn't as readable as the existing library. For me, the most important factors were:
My argument for why this should be merged despite the aesthetics is that it's an issue that users are facing, created by a transitive dependency of a deprecated library inside of a json viewer inside of a debugging plugin for Docusaurus configurations. It's quite a niche area of the website to be visited as it stands and I don't believe that for this particular case, the aesthetics take precedence over dependency conflicts that users are facing in existing installations. |
I've updated the PR to match the styling of the existing JSON viewer. They look much the same now albeit some minor changes. EDIT: I had to rush the style changes to force the types of each imported style but it may be better to just put “as { [key: string] : string }” which may be better. If somebody is able to improve this approach, it would be appreciated. |
@mcrstudio Thanks for the work! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, let's move on!
I'll figure out later how to improve if needed, but it's not high priority.
Thanks 👍
Co-authored-by: Joey Clover <joey@popos.local>
Pre-flight checklist
Motivation
There is currently a peer dependency issue due to the
react-json-view
library depending onflux
, which is no longer maintained and deprecated. If somebody is using rules that require peer dependencies to have no conflict, this is blocking them from upgrading docusaurus.I have migrated the JSON Viewer in the plugin-debug to use
react-json-view-lite
which, despite not having many features, does the job respectfully and has the benefit of working with SSR, whichreact-json-view
did not. This has made the implementation a little tidier.It will also be fairly easy to update
react-json-view
in the future if a similar issue happens and the code for that project is quite a bit simpler.Test Plan
I tested two sites, side by side. One site had the current version of Docusaurus and the other had the version containing my changes. I looked to see if the same properties were collapsed and expanded, which appeared to be the case. This library also appears to be more responsive that
react-json-view
when loading larger depths of expanded JSON content.I'm not sure on how a dogfooding page would work when the task involved removing the old JSON Viewer.
Test links
Deploy preview: https://deploy-preview-9566--docusaurus-2.netlify.app/
Related issues/PRs
Fixes #9486