Automattic / wp-api-console

WordPress (.com and .org) API Console written in React/Redux
GNU General Public License v2.0
70 stars 20 forks source link

Improve the WordPress developer console with a few new features. #86

Closed AllTerrainDeveloper closed 2 years ago

AllTerrainDeveloper commented 2 years ago

While developing some new endpoints in WordPress.com I've realised that I needed to compare data returned from those endpoint and compare them (I was refactoring and needed to be completely sure that the endpoint was returning the same exact amount of information)

With this Pull Request I also wanted to make use of functional component, but seems that the react version we are using does not support it. So I preferred to make the change smaller and not update React at all.

This Pull Request tries to address some of the difficulties I found during that development with the WordPress developer console.

image image

Testing instructions

Check that the default view for every call is still the Tree View. I don't want to disturb others that are used to this view. Check that you can copy the results with the scissors button at the end of the row. You should be able to copy the contents regardless of the selected view Check that regular calls are still working to any other endpoint you were using before (WP .COM & WP REST)

AllTerrainDeveloper commented 2 years ago

I am not sure if I should fix those tests falling, seems they are testing asserts, like assert 2 == 3 for python and another for nodeJS

dmsnell commented 2 years ago

In the picture it looks like the coloration on the tree view/raw view is backwards. I would expect the emboldened or primary color to be on the active option but it looks like the active button blends in with the background.

AllTerrainDeveloper commented 2 years ago

Thank you for your feedback @dmsnell! I've addressed all your concerns + added some comments in the valueRenderer function you were struggling to understand (myself too 🤣 ).

I've updated the Pull Request with the code + new screenshots showing the new colors!

AllTerrainDeveloper commented 2 years ago

I think I've addressed all your concerns with previous issues, I preferred not to use classnames or a similar approach, I've used a single selector instead .selected and reworked a bit the css.

The styling of the JSON view seems visually the same as without the refactor 🙂

AllTerrainDeveloper commented 2 years ago

Sorry to ping you again @dmsnell but I am unable to deploy the changes as it fails compiling in my workstation. Any idea? I've just followed each step on the Field guide, not sure what's wrong in my environment.

Failed to compile.

static/js/main.bfb7c877.js from UglifyJs
SyntaxError: Unexpected token: keyword (const) [./~/jsonpointer/jsonpointer.js:55,0]
dmsnell commented 2 years ago

Eek, looks like we have old dependencies that don't support the more modern ECMAScript features. Probably we should revert the commit and fix the build system.

We have options though:

I'm not sure what our policy is for developer.wordpress.com given that it's public and not internal (we tend to relax our compatibility requirements for internal pages). I'm also away from my computer right now so maybe I can look into this later. I've never worked in this codebase so maybe someone else who has already would know how to update it. You might go poking around and see.

dmsnell commented 2 years ago

After some initial looking I can see we have some old build configurations at play. How about we revert the change for now, update the build system, and then re-apply this patch?

AllTerrainDeveloper commented 2 years ago

I've reverted it. Sorry I've been out for the whole weekend!

dmsnell commented 2 years ago

No worries. Without being able to deploy we didn't break production and this repo doesn't get a lot of activity so I don't think there was any harm.

dmsnell commented 2 years ago

Twist of events @AllTerrainDeveloper

npm run build has been broken since const was introduced in #13 in Nov 2016.

So obviously we're not building this as indicated in the project docs. Maybe @youknowriad might remember something about how it's built. I created #88 to explore moving to esbuild and removing a lot of the implicit config because I couldn't figure out how to easily update babel and webpack and the react-scripts package without breaking everything in different ways.

dmsnell commented 2 years ago

After more investigation I don't think this has been broken since 2016. I believe that an update in the jsonpointer library triggered a breakage at some point recently. Had we pinned all our dependencies with a package-lock.json we may not have noticed this because we still would have been calling for an older version of that dependency which didn't require transpiration.

Proposed fix is to transpile that dependency in #89