Coder Social home page Coder Social logo

react-recollect's Issues

TypeError: Cannot create proxy with a non-object as target or handler

First off, really cool library. I hope it continues to evolve because it's a much more natural way to handle state.

When trying to use the docs, I immediately hit a stumbling block however. I installed recollect into a brand new create-react-app project, imported store and then did:

store.tasks = ['a', 'b', 'c']

And got the following exception:

TypeError: Cannot create proxy with a non-object as target or handler

If I instead do:

store.tasks = []

It works.

This example comes from the readme, so I'm sure I'm not the only one having this issue?

Thanks for your help!

Allow SSR/add resetStore function

If Recollect was used for server rendering, multiple network requests would cumulatively add to the store. Like Redux does createStore for each new render, I should have a resetStore or something like that, that creates a clean version of the store.

In fact, on the server I don't need to add listeners, and can mute the proxy. Think about what a server-rendering mode might look like.

Create ToDoMVC style TODO app

There's not much point actually submitting something to TodoMVC, but doing a CodeSandbox with all the features might be a useful learning tool.

Add Cypress

While I'm at it:

  • organise tests better, split out object/array/map/set
  • tests with hooks
  • Bump React version used in tests

Properties set in componentDidMount aren't registered

If I set store.loading = true in componentDidMount, then fetch some data, then set store.loading = false, the component will never render the loading state.

Problem, the components won't be updated (when store.loading is changed) if they haven't mounted. Or technically, if the HOC hasn't mounted.

Solution one: when triggering an update, don't check if the HOC has mounted, check if the wrapped component has mounted. I don't know how to do this.

Solution two: in the HOC, in addition to tracking if the component is mounted, check if the component is in the initial render cycle. Then, if an update occurs before mounting (of the HOC) is done, still call setState() and let React handle the next render cycle.

Fix sort in < V8 7.0

V8 7.0 changed the way Array#sort works (so the problem is only in Node.js < 11 and Chrome <70)

In V8 6.x and earlier, something funny is going on when I try to sort using a comparison function. I suspect because with each change to the array, I'm updating state.nextStore, and whatever's happening internally to do the sort is losing a reference.

The pre-7.0 behaviour is explained a bit here: https://v8.dev/blog/array-sort#history

The fix will probably be to bypass all proxies for sort, let the engine do the sort (e.g. on a clone array), then update nextStore with the results.

TypeScript definitions are not in NPM release

Hi David, nice idea for a lib & digging your writing style!

Having a go at setting it up with SSR+Typescript and met some resistance:

  1. index.d.ts is missing in the package published to NPM.
  2. store: object causes errors in strict TS
  3. PureComponent is too specific
  4. collect should return ComponentType

How about this:

recollect's index.d.ts:

declare module 'react-recollect' {
  import * as React from 'react';

  interface GlobalState {
  }
  
  interface WithStoreProp {
    store: GlobalState;
  }
  
  interface CollectorComponent extends React.Component {
    update(store: object): void,
    _name: string,
  }
  
  export function collect<P extends object>(Component: React.ComponentType<P>): React.ComponentType<P & WithStoreProp>;
  
  export const store: GlobalState;
  
  export function afterChange(callback: (
    newStore: GlobalState,
    propPath: string,
    updated: CollectorComponent[],
    oldStore: GlobalState
  ) => void): void;
}

User would define interface for their global state in their project:


declare module 'react-recollect' {
  interface GlobalState {
    test?: string[];
    userName?: string; 
  }
}

Disclaimer: not fully tested

Bring all docs into the readme

I've just realised that I hate when people split out readmes so I can't ctrl+f search. Bring everything from /docs into README.md.

Maybe a TOC, and make it clear to the reader that they don't need to know it all, and maybe one day GitBook when I've got nothing better to do.

Better path definition in afterChange

Currently afterChange is called with a string representing the prop path that was changed. E.g. page.tasks.2.done.

This is no good. If tasks was a Map object, it could have one key '2' and another key 2 pointing to separate values.

The internals now use an array for paths, so I'll expose that to users. Will require a major version bump.

Resolved by #69

Spike: can I warn when using global store in a component

Users shouldn't import { store } from 'react-recollect' and the read from that in a component.
I never actually read the first segment of a prop path (called 'store') so I could re-purpose that to be 'next-store'. Then, in the proxy get trap I could check that the path starts with that.

Or maybe I'll have to version them. So, each time I rebuild the store I update the first part of the path so it's store-v1.tasks.1.done or whatever.

But I don't update every prop, so some props will legitimately be old version.s

Maybe this isn't possible at all.

Is there some other way to know 'which' store is being read from? Somehow mix something into the proxy handler? Have a createProxyHandler('v2'). No, that won't work.

I actually don't need to be 100% with the warning. So maybe I can just say for any individual read at a prop-path, if that prop path is not the most recent one, then I can say "hey, looks like you're reading from the global store object inside a component. Don't do that. RTFM".

Fix typo on docs

In your first example:

import React from 'react';
import { collect } from 'react-recollect';
import Task from './Task';

-const TaskList = ({ store ) => (
+const TaskList = ({ store }) => (
  <div>
    {store.tasks.map(task => (
      <Task key={task.id} task={task} />
    ))}
    
    <button onClick={() => {
      store.tasks.push({
        name: 'A new task',
        done: false,
      });
    }}>
      Add a task
    </button>
  </div>
);

export default collect(TaskList);

I'm probably doing something wrong

I've got a component, say <BigFreakingList /> that has hundreds of items.

That then has a bunch of sub components, say <SomeComplexComponent />

Then finally I have <ListItem />

The only component I have wrapped in collect() is <BigFreakingList />.

I have a method called Manager.update(), which I pass down from <SomeComplexComponent /> to <ListItem /> which basically does:

import { store } from 'react-recollect'

class Manager {
  async update () {
    store.items[id].loading = true
    await something()
    store.items[id].loading = false
  }
}

However, when <ListItem /> is triggering update via clicking a button and I have debugOn, I get many hundreds of console log statements even though, I would assume I should just get a few related to updating the one store.items[id] item.

Question is, what am I doing wrong? Should I wrap all components in collect?

My component does the loading update properly on the first pass, then just gets stuck:

EDIT: This is because I was assigning to a variable, making a change, then making another change rather than re-accessing the value from the store.

I'm getting pages upon pages of logs like this:

License?

That's all, this seems neat though.

Add docs for unit testing

For selectors it's easy and probably obvious
For updaters it's easy and maybe not obvious
For components it's easy and medium-obvious

Clarification on using the store in React components

Great plugin, I have been testing it for some days and it works great.
Just a clarification on which store to use with a collected React component (I am a bit confused on this).

As I understood from the documentation a component "reading" from the store should be collected and use the store that the HOC provides in the props.

My question is when this component makes changes to the store (that will affect other components), should it make them to the store from props, the one imported from the plugin or it doesn't matter?
A follow up question is if the component only modifies the store but doesn't read from it, should it be collected? And if so, the first question again, which store to use?

Thanks!

Cannot set store twice in single action

If store.tasks is not defined, this fails:

<button
  onClick={() => {
    if (typeof props.store.tasks === 'undefined') {
      // set the store once
      props.store.tasks = [];
    }

    // expect tasks to be defined
    props.store.tasks.push({ name: 'A new task' });
  }}
>
  Add a task
</button>

Because the first props.store.tasks = [] doesn't update the store before the next line store.tasks.push is called. The order is:

  1. Call props.store.tasks = []
  2. The collect HOC has setState called with the new store. But, React will wait for the other synchronous code to finish before triggering a re-render, so...
  3. The event handler code continues, but props.store hasn't yet been updated (React is waiting for this code to finish) so props.store.tasks is still undefined and props.store.tasks.push() fails.

Using the global store instead of the one passed in as props resolves this, but it's not great DX to have the rule "when reading from the store, use props.store, when setting in the store, use the global store".

And what if I want to do two things with the passed in task object? I can't get a reference to this in the global store.

Is it time to name them differently? Does that help?

Approach to updating deeply nested data?

deep references to items in the store may be broken if you modify the store. I'd be interested to hear about cases where this is proving unpleasant. Please feel free to open an issue with a code snippet, even if you think it's something that can't be fixed.

Me! So, I have an object tree like: store.devices[id].services[id].something

I have to, because of possible null/undefined values, check to see that the particular item exists when doing an update operation.

Right now, I am doing something kinda ugly:

// not actual code, but similar

if (!store.devices[dID]) throw new Error('No device!')
if (!store.devices[dID].services || !store.devices[dID].services) throw new Error('No services!')
if (!store.devices[dID].services[sID]) throw new Error('No service!')

store.devices[dID].services[sID].foo = true
await something()
store.devices[dID].services[sID].foo = false

Is there a better way to do this?

What if my object was store.devices[dID].services[sID].foo[a].bar[b].baz = true?

Question: How would you create "selectors"

Would using something like reselect be possible with recollect? For example, creating a selector that filters a list of items in your store and that automatically updates when the store changes?

Is there another way to approach this?

Thanks and keep up the good work!

Add tests for isolation

Add a test scenario that ensure changes to one part of the store don't trigger updates in components that never read from that part of the store (e.g. a chat section in the tasks page)

Prevent update if from/to values are the same

I should be able to short circuit some work by checking if 'from' and 'to' are strictly equal in set().

E.g. if a task is done, setting task.done = true shouldn't even trigger an update.

Is there a scenario where this is bad? Would two strictly equal prev/next values ever need to trigger an update? React would always just bail during the dom diff anyway, right?

Note: there's a spot where I mention this in updater-patter.md - that will need updating.

Getting more people to use recollect?

What are you plans/thoughts/ideas to get more people to use recollect? I think it's got a lot of promise and would be good to see some more activity!

Remove Babel (investigate)

If I use React.createClass and require instead of import then I can maybe do away with Babel.
Is there any point to this?

Reasons not to:

  • I run the risk of accidentally doing a trailing comma in a function's argument list or something that will blow up somewhere.
  • Babel can strip out comments which is nice
  • Not sure how imports into a user's es6 module file work if my modules are commonjs.

Bundle with Webpack or Rollup

Using plain Babel is OK, but I think it has problems with circular references.

In general.js:

export const TEST = 'hello';

and in debugger.js

import { TEST } from './general';
console.log(TEST);

And I get
image

Because debug imports general, which imports proxy, which imports proxyHandler, which imports collect, which imports store, which imports debug.

Resolve circular dependencies

And general tidy up

Notes for release:

Breaking changes:

  • Prop paths no longer have the superfluous store. prefix.
  • afterChange event has updatedComponents and changedProps properties replacing components and propPath
  • Updating the store during a render (technically, a collection cycle) results in an error. This includes code in componentDidMount().

New stuff:

  • batch

RangeError: Maximum call stack size exceeded

Why does the associated component always seem to get this error

RangeError: Maximum call stack size exceeded

whenever i want to set a new value for an array store property. Please, check the code snippet below
axios.delete('/v1/order/deleteorder/' + id) .then(res => { let orders = this.props.store.orders; this.props.store.orders=orders.filter(({_id})=>{ return _id !==id }) // this.props.history.location('/order') })

I wish to filter the store.order and set new value after deleting an order but i get that error.
Please do let me know what i am doing wrong.

Not SSR safe

Rendering a component with store server side ends in an explosion:

ReferenceError: localStorage is not defined

Note: I guess that on server, store could be just a plain object, no need for proxy.

Update afterChange parameters to be an object

I think afterChange has too many parameters. As it turns out, it's super useful for testing, but I have to do this:

expect(afterChangeHandler.mock.calls[0][1]).toBe('store.loading');
expect(afterChangeHandler.mock.calls[0][0].loading).toBe(true);

This would be nicer:

const storeChange = afterChangeHandler.mock.calls[0];
expect(storeChange.path).toBe('store.loading');
expect(storeChange.store.loading).toBe(true);

And the usual case would change from this:

afterChange(store => {
  localStorage.setItem('site-data', JSON.stringify(store));
});

To this:

afterChange(({ store }) => {
  localStorage.setItem('site-data', JSON.stringify(store));
});

Proposed signature would be

afterChange(({ store, propPath, prevStore, components }) => {});

This would be a breaking change, so v3.

Docs would need to be updated.

Better Map() and Set() support

store.set = new Set();
const someObject = {prop: 'value'};
store.set.add(someObject);
someObject.prop = 'new value'; // Doesn't trigger a render

This is surprisingly complicated. If a user stores an item in a set, and passed that item to a component to render, then changes the content of that item, the item will be cloned (nothing is mutated). But everything in the store has a path, and for a Set, the value is the path. But the value changes! The issue is the same for Maps where the key is an object.

Also, a change to the key of a Map (assuming the key is an object) should update any component using that key. But the key is part of the path, and changing it will clone it, so the path breaks.

One option is to use indexes in prop paths, rather than the value. This is less readable to the user, but reading from the path is probably fairly rare.

Another (potential) option is to use a WeakMap to maintain a reference between an object and its clone.

Add index.d.ts

Or maybe a @types/react-recollect thing. Learn how all this works, what do the editors pick up?

Question: Would this work with Stencil?

This is sort of a random question, but would this work with the Stencil framework?

I've been playing with Stencil, which creates Web Components with a very similar API to React:
https://stenciljs.com

I gave this a shot but it seems to fail on an import so I can't test this.

[ ERROR ]  Rollup: Missing Export: src/components/app-home/app-home.js:1:9
           'store' is not exported by
           node_modules/react-recollect/dist/index.js

Would be interesting to see if recollect could be made agnostic of React?

DevTools

Before you ask... I read the readme.

Use __RR__.debugOn() to turn on debugging. The setting is stored in local storage, so will persist while you sleep. You can combine this with Chrome's console filtering, for example to only see 'UPDATE' or 'SET' events. Who needs professional, well made dev tools extensions!

Yes, this is awesome. EXCEPT, Google freaking ripped out the filtering feature in the console read more. So that really makes debugging in the console very difficult.

Any plans on reusing the popular redux dev tools to show changes in the store? MobX does this, and it works really well.

If you don't have time, just let me know and I may be able to spend some time looking into it.

Lastly, this is a really great little library. I have already used it on two of my personal projects. Thank you for your hard work!

Spike: smarter updating of listeners

Part 1 - parent path listeners

Currently, if there are two listeners:

  • <ParentComponent> listening to store.tasks
  • <ChildComponent> listening to store.tasks.1.done

Then if the prop at path store.tasks.1.done is updated, then both components are updated. I did this for a specific reason in the early days and really should have documented why.

Edit, this is because a child component will often not get its data from the store passed by collect() - it will get the data passed from the parent (e.g. a <TaskList> will pass each task object from an array to a child <Task> component). So if I don't update the <TaskList> component with the new store, it wouldn't know if a task was ticked.

At the very least I need to add a test to protect/document this.

Part 1 result: no change to code, but a change to guidance:

"You don't need to wrap a component in collect unless you want access to the store in that component". Two reasons for this:

  • If <TaskList> references store.tasks and renders a bunch of <Task> components, and the <Task> component is not wrapped in collect, then all of the reads in the <Task> components (store.tasks.0.name, etc) are attributed to the <TaskList> - so it's listening on everything its children rendered.
  • And a second reason I've just forgotten.

Also, if you aren't wrapping your components in collect() make sure you're using PureComponent or React.memo. To not use these is to be slow for no reason.

Part 2 - child path listeners

I also update down the prop tree. E.g. if a prop store.tasks changes (an entire array is overwritten with a new array), then I'll trigger updates on components listening to paths that start with that. So, store.tasks.0, store.tasks.0.done and so on.

In other words, if you're listening to any prop 'inside' a prop that changes, you need to know about that parent prop changing. But, if a listener is listening to a child prop and is a child component then it won't need to update, because the parent component will update. But I have no way of knowing the relationships between components.

I don't think there's much I can do here. And, I think this falls under the control of React, which will not be wasteful even though I've requested redundant updates.

Part 2 Result: no change

Part 3 - tracking previous value

What if, for each listener, I stored the last value at the prop path a component is listening to? Then, when updating, if the value hasn't changed, don't bother updating. Would this catch anything?

  • For updating listeners with an exact match on the path, it makes no difference (the proxy handler won't trigger an update if the value didn't change).
  • For listeners to a parent prop path, it won't matter, because if a value changed for any prop path, then all parent objects in the store will be different (because immutability).
  • But for listeners to a child path (I overwrite store.data and some component is listening to store.data.page.title), then if the title didn't change, I don't need to update it. But a component listening to store.data.page.title must also be listening on store.data.page and that will be a new object.

Also, would this take a lot of memory?

Part 3 Result: no change

Part 4 - listening on objects and arrays

Here's a question: do I need to listen to props that are objects/arrays? (And I mean {} objects, not null, etc). You can't actually use an array or an object without accessing one of its child properties.
I mean, you can't render a task to the screen. You can only render task.done and task.name and task.id and so on. Even if you called JSON.stringify(task) that calls everything under the hood.

Hmmm, I think I do need to listen to objects/array, because of part 1 above. I might only read an object in a parent component (store.page), then pass that object to a child component (regardless of whether it's wrapped in collect or not) and it reads a property that's a string page.title. So yeah, I need to be listening for changes to the object, because Recollect might not be aware of child components listening to child props.

Part 4 result: no change


Well this is good and bad. I think it's about it's efficient as it can be regarding updating, unless I can think of a way to not trigger an update on a child component if I'm updating a component further up the tree (and only bother if React is handling this).

Batch component updates (maybe)

This could have large implications. For one, all tests that change the store and check for a response would need to be async.

But what of non-tests. Is this likely to case trouble in userland? If React itself is headed toward async updates then maybe this doesn't have such a big impact, negative or positive.

Suss out the scenarios...

Resolved by #73

Pave the way for time travel

I don't want to implement time travel, but want to allow middleware to. I'm thinking something like this:

  • currently I pass the next store to afterChange. I could also pass the previous store.
  • I can pass an array of React components that were updated
  • Someone could write middleware to be used like afterChange(someRewindLibrary) that exposes back() and forward() methods (e.g. in the console)

I think that'll work. I need to have a think about whether just passing the old store back to the components that updated is enough. I can't think why it wouldn't be.

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    ๐Ÿ–– Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. ๐Ÿ“Š๐Ÿ“ˆ๐ŸŽ‰

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google โค๏ธ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.