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

Correctly Serialize Maps #496

Closed
ZackingIt opened this issue Jan 2, 2020 · 17 comments
Closed

Correctly Serialize Maps #496

ZackingIt opened this issue Jan 2, 2020 · 17 comments

Comments

@ZackingIt
Copy link

ZackingIt commented Jan 2, 2020

Happy to open a PR on this myself, want to clear with maintainers:

Map objects are not displaying any key-value pair contents. For example, our Redux store actually contains a very large Map object with a lot of data, but none of it shows up here:
Screen Shot 2020-01-02 at 11 38 10 AM

Whereas when we console.log the data it does in fact serialize correctly and display. I would be happy to open a PR on this and write the serializer myself -- this makes Redux dev tools much less useful when I use the Map object.

@erezcohen
Copy link

erezcohen commented Mar 7, 2020

True.
This is, BTW, a regression from some previous version.
Maps used to be serialized, although as I recall they were displayed as an array of arrays.
Would be nice to see it displayed as chrome dev tool shows them.

@Methuselah96
Copy link
Member

@ZackingIt This seems to be working for me (here's a quick demo: https://methuselah96.github.io/redux-devtools-app-testing/). Can you create a reproducible example?

@Methuselah96
Copy link
Member

Nevermind, I was just testing it with react-json-tree. I see now that it doesn't work in the Redux DevTools. I'll take a look at it later.

@Methuselah96
Copy link
Member

Methuselah96 commented Apr 30, 2020

It's worth noting that Redux highly discourages putting Maps in Redux state (https://redux.js.org/style-guide/style-guide#do-not-put-non-serializable-values-in-state-or-actions). I might look into it later, but it's not a high priority because you really shouldn't be putting Maps in state in the first place.

@erezcohen
Copy link

Ha. You are right. I was aware of the rule of non-serializable values in the store, but I was not aware that Maps and Sets are included there. And indeed, I looked at the history of that style guide in github I saw that the words "Maps/Sets" were added to the sentence just 4 months ago.
So I guess we won't be adding any new Maps or Sets to the store.
But it won't be easy to refactor out all the existing ones...

@rob2d
Copy link

rob2d commented May 3, 2020

It's worth noting that Redux highly discourages putting Maps in Redux state (https://redux.js.org/style-guide/style-guide#do-not-put-non-serializable-values-in-state-or-actions). I might look into it later, but it's not a high priority because you really shouldn't be putting Maps in state in the first place.

I can get the rationale behind non-serializables (to pass around JSON), but it seems like this can be worked around with various quick/compact libraries for formatting and supporting that (for example: https://www.npmjs.com/package/typeson). Also, it's definitely a maintainers right to insist on that, but there are benefits to using Maps/Sets. Non exhaustive list: in newer browsers without polyfill, it's much faster to iterate through Map/Set entries -- also the data structures are idiomatic and some problems simply are better suited to app logic itself -- especially Sets. Objects can take the place of set but key namespaces are a little bit limited to types (e.g. Boolean true/false vs string), and you cannot use shallow references as keys, etc.

I notice that there hasn't really been any updates on this library also; wondering if maybe the maintainers have moved on (which I can completely empathize with). If that is the case, it could be worth forking the dev tools since it's not really the most technical problem outside of backwards compatibility with exported JSON and some other stuff. But also if we want some functionality like this, it probably shouldn't be on the burden of the maintainer if it solves their own problems and 90% of users' scenarios (as much as I hate to say that as someone who loves to use Sets regularly).

@rob2d
Copy link

rob2d commented May 3, 2020

Ha. You are right. I was aware of the rule of non-serializable values in the store, but I was not aware that Maps and Sets are included there. And indeed, I looked at the history of that style guide in github I saw that the words "Maps/Sets" were added to the sentence just 4 months ago.
So I guess we won't be adding any new Maps or Sets to the store.
But it won't be easy to refactor out all the existing ones...

Yeah this is sort of a problem with JSON being so closely linked to JavaScript. It sucks that there hasn't been a new format that is widely adopted natively for JS. But as mentioned above there's some nice libraries that are fast and with minimal overhead to JSON; I personally had really good experience for Maps/Set encoding w typeson in the recent past:
https://www.npmjs.com/package/typeson

Edit: misremembering Typeson as TSON

@Methuselah96
Copy link
Member

Methuselah96 commented May 4, 2020

@rob2d It is true that the former maintainer of this project has moved on (see #502). Up until this past week I've been too busy to dedicate any time to this, but starting last week I now have quite a bit of free time for the foreseeable future and hope to be an active maintainer for this project. I've just started to get familiar with the code in this library and am working on multiple PRs at the moment.

All the write privileges for this repo and corresponding packages have been given to @timdorr, but I also understand that he is quite busy. Hopefully he has enough time to review/merge my PRs and publish them somewhat frequently. If that ends up falling through then I'll end up forking the project and releasing the packages under a different name, but that would be unfortunate and hopefully unnecessary.

Regarding correctly serializing Maps and Sets in the Redux DevTools: @erezcohen said they used to work in the past. If that's true, then there's probably already a system in place to serialize and deserialize them correctly, and there just happens to be a bug or something else got messed up. As I become acquainted with the code, I'll see if I run across any code that is already meant to handle Maps and Sets and take it from there.

@timdorr
Copy link
Member

timdorr commented May 4, 2020

I'm around! Let me add you to the project as a contributor. I'll do that in a bit when I'm at my computer.

@joshburgess
Copy link

At one point in time, about 1-2 years ago, all that was required to make something serializable by the dev tools was to patch in a toJSON method that returned valid JSON.

I once made a custom data structure, SerializableMap, that extended Map and just defined an extra toJSON that returned the key-val pairs in an object representation for easy viewing in the dev tools. It worked perfectly. Not only did it display the key-val pairs in the dev tools legibly, but it was actually, arguably, more readable than how a plain Map serializes in the console.

However, this no longer seems to work.

If someone chooses to fix the bugs with Map & Set, it might be nice to go just a little bit further and re-implement some kind of external support for custom data structures like the above. After all, there could very well be people out there using their own immutable data structures, like hand-rolled HAMTs, linked lists, etc.

If the user only needed to provide a single method, like toJSON, serialize, printToDevTools, or something along those lines, and the library simply called it without knowing anything else about the data structure, and things just worked, that would be ideal.

Essentially, this would provide extensible support for serializing any custom data structure just by establishing (and documenting) a simple protocol that the user must implement. I think it's worth considering.

@Methuselah96
Copy link
Member

Methuselah96 commented Aug 6, 2020

Turns out that there is an option to enable serializing non-JSON objects (serializing Maps is not enabled by default). See the serialize option (set options to true) in order to enable serializing Maps for the Redux DevTools extension.

Let me know if that works for you. Closing for now as I believe this is resolved.

@steinarb
Copy link

steinarb commented Aug 7, 2020

Let me know if that works for you. Closing for now as I believe this is resolved.

Doesn't work for me, unfortunately, since I use reduxjs/toolkit to set up the store, and setting up redux-dev-tools is hidden inside reduxjs/toolkit.

@steinarb
Copy link

steinarb commented Aug 7, 2020

I've opened reduxjs/redux-toolkit#683 to have redux-toolkit set the serialize option when setting up redux-devtools

@Methuselah96
Copy link
Member

@steinarb You should be able to set it up in configureStore.devTools like so:

export default configureStore({
  reducer: {
    // TODO,
  },
  devTools: {
    serialize: {
      options: true
    }
  }
});

@steinarb
Copy link

steinarb commented Aug 7, 2020

I was able to set serialize using devTools parameter to configureStore, like so:

const store = configureStore({
    reducer: createRootReducer(history),
    middleware: [
        sagaMiddleware,
        routerMiddleware(history),
    ],
    devTools: { serialize: { options: true } },
});

Well, maps looks different than before... (we create a new version of our application each year and in the 2019 version Map still showed up in redux toolkit).

The old way was to show map as an object containin an array of two element arrays.

The new way is to show Map as "> Iterable" and when you click on the arrow, the keys show up like when opening a regular JS object.

I guess the new way is better.

@Methuselah96 Methuselah96 removed the bug label Aug 7, 2020
@jochumdev
Copy link

This works for me:

const makeStore = () => {
  const sagaMiddleware = createSagaMiddleware();

  const store = configureStore({
    reducer: rootReducer,
    middleware: (getDefaultMiddleware) => getDefaultMiddleware({thunk: false}).concat(sagaMiddleware).concat(routerMiddleware),
    devTools: process.env.NODE_ENV !== 'production' ? { serialize: { options: { map: true, }, }, } : false,
  });

  sagaMiddleware.run(rootSaga);

  return store;
}

@KovDimaY
Copy link

Does somebody know why this serialize configuration is disabled by default? Does it have any drawbacks to having it enabled? Thanks in advance!

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

No branches or pull requests

9 participants