Complex objects unexpectedly represented as strings

[rufusraghunath]

Hi @zalmoxisus, thanks for this fantastic extension! I've been using the redux-devtools-extension for around 1.5 years and found it really useful.

One thing that has always bothered me is that complex objects appear to be represented as strings. For example, when storing a Date in the state, the devtools show you its toString() rather than the object itself. This can make it hard to know whether you've persisted the correct type of data into the store, which has helped create a few bugs for me in the past. I'd much rather be shown a plain object (even though that would be more messy) and only see a string when the data in the store is actually a string.

If there isn't already an established solution to this, would you be open to me submitting a PR?

Thanks for your help!

[zalmoxisus]

Sorry about the late reply, going now through the backlog. This should be implemented on monitors part, either log monitor or inspector. Or we can handle that in Raw View tab, which is intended for that. As you can see here, it uses javascript-stringify to show the data.

[rufusraghunath]

Thanks @zalmoxisus, I'll take a look this week. If it's OK with you I'll send a PR and we can discuss. Feel free to assign this issue to me :)

[rufusraghunath]

Hey @zalmoxisus, I'm looking at this now. I've noticed that the tests seem to be failing for at least two of the devtools repos. I've just done an npm install and am running npm test. Am I setting something up wrong?

Here is the output for remotedev-app:

● App container › should contain an empty action list

    expect(received).toMatch(expected)

    Expected value to match:
      /<div class="actionListRows-[0-9]+"><\/div>/
    Received:
      "<div class=\"actionListRows-0-4\"></div>"

      at Object.<anonymous> (test/index.spec.js:29:5)
          at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:188:7)

  App container
    ✓ should render inspector monitor's wrapper (5ms)
    ✕ should contain an empty action list (4ms)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 passed, 2 total
Snapshots:   0 total
Time:        9.942s

And here is the output for redux-devtools-extension:

1) App container should contain an empty action list:
     Error: Expected '<div class="actionList-0-2"><div class="actionListHeader-0-3"><input class="actionListHeaderSearch-0-16" placeholder="filter..."><div class="actionListHeaderWrapper-0-17"><div class="rightSlider-0-40"><div class="actionListHeaderSelector-0-5"><div class="selectorButton-0-30 selectorButtonSmall-0-31">Commit</div></div></div></div></div><div class="actionListRows-0-4"></div></div>' to match /<div class="actionListRows-[0-9]+"><\/div>/
      at assert (node_modules/expect/lib/assert.js:29:9)
      at Expectation.toMatch (node_modules/expect/lib/Expectation.js:138:28)
      at Context.<anonymous> (test/app/containers/App.spec.js:19:7)

It looks to me like the assertion might be set up incorrectly. Currently the expectation is .toMatch(/<div class="actionListRows-[0-9]+"><\/div>/);. But looking at the format of the actual class name, the expectation should be .toMatch(/<div class="actionListRows-[0-9]-[0-9]"><\/div>/);. I'm not great at regex, but I think [0-9]+ doesn't work because the numbers are hyphen separated, so you need two separate character sets. Adjusting it to this fixes the tests for me.

[rufusraghunath]

Do we have a contributor guide somewhere that outlines architecture and testing practices? The only docs I could find were instructions on how to use the devtools.

[zalmoxisus]

Hey @rufusraghunath thanks for working on that. Looks like jss inside redux-devtools-instrument changed the way styles are represented. That shouldn't affect anything except tests. You can use yarn instead of npm install which locks subdependeces. We could add that in README, it was written before yarn appeared. Or could fix that test and update.

A contributor guide is a good idea, I'll work on that after moving everything into a monorepo to make it easier for new contributors, now we have everything split into more than 20 repos.

Regarding this issue, you'd want to check it in javascript-stringify (it's used for representing raw data object of action/state, for Diffs and Test tab) and in react-json-tree (it's used inside the monitors to represent action/state object tree).

Inside the extension we're using jsan for serialization, and it supports Date (not converting into string), I've checked:

jsan.parse(jsan.stringify(new Date(), null, null, true))

[rufusraghunath]

Great, I definitely think there'd be some value to a guide... I've been spending quite a lot of time just reading through the different repos trying to understand how they are connected and what the responsibility of each is. That could be made easier with some architectural notes. Thanks for the pointers. I'm going to try to reproduce the issue and then trace where it comes from. I've worked on a Lerna migration before, so let me know if you'd like some help with the monorepo effort. It definitely involves some complexity, and an extra pair of eyes never hurts :)

…

On Thu, Nov 29, 2018, 06:13 Mihail Diordiev ***@***.*** wrote: Hey @rufusraghunath <https://github.com/rufusraghunath> thanks for working on that. Looks like jss inside redux-devtools-instrument changed the way styles are represented. That shouldn't affect anything except tests. You can use yarn instead of npm install which locks subdependeces. We could add that in README, it was written before yarn appeared. Or could fix that test and update. A contributor guide is a good idea, I'll work on that after moving everything into a monorepo <reduxjs/redux-devtools#412> to make it easier for new contributors, now we have everything split into more than 20 repos. Regarding this issue, you'd want to check it in javascript-stringify <https://github.com/blakeembrey/javascript-stringify> and in react-json-tree <https://github.com/alexkuz/react-json-tree>. Inside the extension we're using jsan for serialization, and it supports Date (not converting into string), I've checked: jsan.parse(jsan.stringify(new Date(), null, null, true)) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#468 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AOkarMUcPBdDUQOBYP5xcAAlJ4jI4brpks5uz8FegaJpZM4Sfzpp> .

[zalmoxisus]

That would be great if you can help. If you have time to work on this, a pr on redux-devtools would be welcome or I'll start that these days and we can continue the discussion there. It will be slow gradual process to migrate all the repositories, but can start with redux-devtools there. After that I'll make PRs for every repository to keep all the contribution history in the new one. For the extension we'll keep the current repository as a legacy for 2.x version, while creating 3.x in the monorepo.

[rufusraghunath]

Ok cool, so you want to create a new repo for the monorepo, rather than adding Lerna to an existing repo? How will we be able to keep the contribution history? Does Github provide some way to do that? Go ahead and do the first one, and I'll see if I can pick up some other repos after that. Maybe tag me in your PR for redux-devtools when you make it so I can get some context? We can discuss more there.

…

On Thu, Nov 29, 2018, 13:45 Mihail Diordiev ***@***.*** wrote: That would be great if you can help. If you have time to work on this, a pr on redux-devtools <https://github.com/reduxjs/redux-devtools> would be welcome or I'll start that these days and we can continue the discussion there. It will be slow gradual process to migrate all the repositories, but can start with redux-devtools there. After that I'll make PRs for every repository to keep all the contribution history in the new one. For the extension we'll keep the current repository as a legacy for 2.x version, while creating 3.x in the monorepo. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#468 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AOkarITD_l1BDrBvjqE-6FP6zUdL5bCBks5u0CsxgaJpZM4Sfzpp> .

[zalmoxisus]

No new repo, the idea is to transform redux-devtools into a monorepo, note that is different from current redux-devtools-extension repo. The former is a React Component to include monitors inside the app, not browser extension. But everybody confuses it, so better to join it together. Since that's already inside Redux org, it looks like better host than the current one.

About the history for other repos we want to move, there's no much work there, I'll just create a new branch or fork, move everything into a new packages/name folder, then instead of doing a PR to master branch of that repo, change upstream for redux-devtools, then it won't be squashed and we'll get all the history inside. I think that should work. Then that repo should be either archived or just with a mention and the link in the README that it was merged to redux-devtools monorepo. NPM packages remain the same, so original authors still keep ownership.

I'll change the structure and try that, then will ping you in that PR if want to help with lerna config.

[zalmoxisus]

I fixed a regression in jsan which was causing this issue, and now Dates are handled well for state/action tree and raw view:

It works in latest version on Chrome Store.
I found only 2 problems:

• The diffs shows only when it's changed from other type to Date, but don't compare dates.
• Also Chart monitor doesn't show Date type.
  I'll look into these later as anyway wanted to handle "non-stringified" types in the tooltip.

About monorepo, I wasn't right, that idea would work if either redux-devtools or the repo we want to merge in would have originally that structure with packages. But now we'll have conflicts within history, and by solving them everything goes in one commit. So I guess no way to move all the history.

If you'll find time to look into setting lerna and do a PR against monorepo branch for those 2 packages, that would be much appreciated. I'll gradually merge other packages.

[rufusraghunath]

OK, great, that's really helpful. In that case, I think we'll be able to close this issue oncethe two follow-up items are done?

About monorepo - I'm happy to put in some work over the next month, but I'll need some more information.

• Is there another place we can communicate? (Especially if we're going to be closing this issue).
• Do we have a deadline for this, or are we flexible?
• I think it makes the most sense if we take a gradual approach and first get Lerna set up in redux-devtools with a single dependency in /packages, and then we can make PRs for each of the other repos. Sound good? Do you have a preference for which package to start with?

I also have some basic questions around contributing to this project:

• How do you test your changes? Do you just run the app locally and test manually?
• In that case, how is that done, and how do you set up test data?
• What kind of unit test coverage are we aiming for?
• Is there a CI process that runs the tests?
• Is there a document somewhere that lists all the repos in the redux-devtools ecosystem and summarizes their role? This would be extremely helpful for me when working on the monorepo migration, since I'm coming in as a new contributor.

I think we would really benefit from a contributor guidelines document 👍

[zalmoxisus]

I made a minimal implementation just few hours ago. I didn't rely on lerna much, using just yarn workspaces and made a node script to run linting and tests inside packages. Probably would be great to have the same rules and dev dependences among packages, but let's keep it after moving them all. Still unsure about publishing and how to proceed with release tags, redux-devtools is at 3.x while it will be just a package and release tags should be mainly about extension, which is 2.x.

Is there another place we can communicate? (Especially if we're going to be closing this issue).

I'll publish a PR now, so we can continue discussing there. For now we just need a minimal configuration, so I can move packages slowly there.

Do we have a deadline for this, or are we flexible?

We can improve this later, I'll just move repos gradually. If you'll have time PRs on improving that would be much appreciated.

I think it makes the most sense if we take a gradual approach and first get Lerna set up in redux-devtools with a single dependency in /packages, and then we can make PRs for each of the other repos. Sound good? Do you have a preference for which package to start with?

Agree, we have 2 packages there, which is the one which repo we are using and I also moved instrumentation, which is the main part of redux devtools (it was actually moved from there few years ago).

How do you test your changes? Do you just run the app locally and test manually?
In that case, how is that done, and how do you set up test data?

The plan is to have most of functionality decoupled in other packages and have unit tests and snapshots for React components there. While for chrome/firefox extension having mostly e2e tests.

Currently devpanel loads the same script as for extension window, so we are just testing that page with selenium. Just if all monitors rendered and if we can get the data from the client part, for which we are using a http://zalmoxisus.github.io/examples/router/ example. Probably better using an example so it can work offline, just was an extra step for CI to install that, but surely better to go that way. The rest goes to unit tests.

In next version devpanel page will inject the script directly (in order to be used not only for a browser tab), so it needs to stay in Chrome devpanel. I'm exploring puppeteer for that, but it's still limited for Chrome DevTools extensions. Most likely we'll just test inside an Electron app like doing it now.

Most of the code is pretty old, I removed gulp and made a lot of things easier in remotedev-app. The extension will come the last, we need to move the rest components in monorepo for now.

Not sure if moving all the tests in the root part of monorepo instead of having tests inside for every package is better.

What kind of unit test coverage are we aiming for?

The most sensitive part is instrumentation, because it wraps all Redux part even if the extension is not in use (not opened extension devpanel or window), so it can break users apps, since many sites are leaving __REDUX_DEVTOOLS_EXTENSION__ enhancer in production. So we aim for as near to 100% as possible there.

As for the extension part, it has a lot of experimental features, which are not even documented. More tests weren't added since the beginning, so the situation is not quite plausible here. The goal is to move most of injected part outside with good coverage there, while react components should be covered in monitors packages and remoedev-app. Here we'll have e2e tests to check if the extension loads and if it interacts with the page loaded in the browser.

Is there a CI process that runs the tests?

All repositories are using Travis. For the extension (this repository) we're also using Appveyor for e2e tests on Windows.

Is there a document somewhere that lists all the repos in the redux-devtools ecosystem and summarizes their role? This would be extremely helpful for me when working on the monorepo migration, since I'm coming in as a new contributor.
I think we would really benefit from a contributor guidelines document 👍

Yes, we need such document. I'll work on it.

UPDATE: Let's move it to reduxjs/redux-devtools#416

[rufusraghunath]

OK great, I'll head over to redux-devtools and check out what you've done. I'll comment on the PR there :)