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

Discussions about rest operator #8

Closed
dai-shi opened this issue Mar 19, 2019 · 32 comments
Closed

Discussions about rest operator #8

dai-shi opened this issue Mar 19, 2019 · 32 comments

Comments

@dai-shi
Copy link
Owner

dai-shi commented Mar 19, 2019

from: #1 (comment)

rest operator

const { someKey, ...rest } = someObject
<div>rest.someValue</div>
@dai-shi
Copy link
Owner Author

dai-shi commented Mar 19, 2019

@theKashey said:

With this approach, you are just "reading" from state inside component, as you wanted to do, and no need to rest.

We still want to support the rest operator if possible. Or at least provide a workaround.

I haven't looked into proxyequal's spread guards. Is it related to the rest operator?


So far, what I could think of is the following.

const used = { current: {} };

const PROXY_REST = Symbol('PROXY_REST');

const handler = {
  get: (target, name) => {
    if (name !== PROXY_REST) {
      used.current[name] = true;
    }
    return target[name];
  },
};

const restHandler = {
  get() {
    return (excludingKeys) => {
      const obj = this; // I didn't expect this works. Is it stable??
      const clone = {};
      Object.keys(obj).forEach((k) => {
        if (!excludingKeys.includes(k)) {
          clone[k] = obj[k];
        }
      });
      return createProxy(clone);
    };
  },
};

const createProxy = (obj) => {
  const proxy = new Proxy(obj, handler);
  Object.defineProperty(proxy, PROXY_REST, restHandler);
  return proxy;
};
const s = { a: 1, b: 2, c: 3, d: 4 };
const p = createProxy(s);

// const { a, b, ...rest } = p;
// ...transpile the above to the below by babel or by hand.
const { a, b } = p; const rest = p[PROXY_REST](['a', 'b']);

console.log(a, b, rest.c);

console.log(used);

@theKashey
Copy link

I was thinking about something like

const { a, b, ...rest } = p;
// ...transpile the above to the below by babel or by hand.
const { a, b } = p; 
const rest = WITHOUT_TRACKING(p, () => 
const { a, b, ...rest } = p; 
return rest;
});

Your variant is a bit more compact, but I am not sure which one is easier create using babel. @faceyspacey - have you started anything?

@faceyspacey
Copy link

faceyspacey commented Mar 20, 2019

@ScriptedAlchemy has successfully implemented this in Babel:

//INPUT:
export const MyComponent = (props, state, actions) => {
  return <div>{state.title}</div>
}

// --> OUTPUT:

// rename the original component
const MyComponentOriginal = (props, state, actions) => {
  return <div>{state.title}</div>
}

// and then simply use it as a function within the template:
export const MyComponent = (props) => {
  const { state, actions } useRespond('__respond__currentFileName') // for our "Redux Modules" capability
  return MyComponentOriginal(props, state, actions)
}

useRespond uses useReduxState and useReduxDispatch under the hood

https://github.com/faceyspacey/remixx/blob/master/src/babel/helpers.js#L84-L94

We are trying not to touch the work ofreact-hooks-easy-redux, and see how far we can go with our tasks on the babel side and the work of this lib as a dependency.

We got a bit more work at this stage, but we are going to attempt the useMemo + babel approach that @theKashey recommended. This is the approach where we detect what keys on state are used (and if they are selectors) in order to pass as dependencies to useMemo. Then of course we gotta fork Redux and allow for passing in selectors, plus the work related to namespacing for "redux modules."

That is to say, we got our hands full, but we'll take the assignment of selectors. We are hoping to depend on this library for good perf/benchmarks + rest operator. And we will give @dai-shi credit and mad "props" everywhere we go as we promote our tools. @dai-shi perhaps you want to join our unofficial team of collaborators. We are a rag tag decentralized army of gorilla warfare warriors, combatting the status quo 😎

@theKashey
Copy link

t-shirt, or at least stickers to share?

@faceyspacey
Copy link

t shirts maybe, stickers are in the works

@dai-shi
Copy link
Owner Author

dai-shi commented Mar 20, 2019

const rest = WITHOUT_TRACKING(p, () => 
const { a, b, ...rest } = p; 
return rest;
});

Does this help? We want to track rest afterward, right? @theKashey

you want to join our unofficial team of collaborators.

Sure. Stickers!

@theKashey
Copy link

theKashey commented Mar 20, 2019

Does this help? We want to track rest afterward, right? @theKashey

You will get the proxy tracker as a rest, but you will not track that "get".

@dai-shi
Copy link
Owner Author

dai-shi commented Mar 20, 2019

console.log(a, b, rest.c);

I assumed that affected would be ['a', 'b', 'c'] in this case. And it is so in my example.

@theKashey
Copy link

The same should be for mine - tracking is off only when you are extracting rest from the source object - later it's on again.

You approach would not work for one (edge) case - nested proxies, as long as it still would read from underlaying proxy. My approach just killing tracking, at all, giving you do that every you wanna to, and turning it back. I've used another syntax just to make babel plugin simper (just wrap). Read it like

const REST = (source, excludingKeys) => {
      const clone = {};
      Object.keys(obj).forEach((k) => {
        if (!excludingKeys.includes(k)) {
          // reading from proxy!
          clone[k] = obj[k]; // would keep refs to the underlaying proxy-objects
        }
      });
      return createProxy(clone);
};


const { a, b } = p; 
DISABLE_ALL_TRACKING(); const rest = REST(p, ['a', 'b']); ENABLE_ALL_TRACKING();

@dai-shi
Copy link
Owner Author

dai-shi commented Mar 20, 2019

      const obj = this; // I didn't expect this works. Is it stable??

I wasn't sure if this is correct. Maybe, I need to try nested proxies. Will try.

DISABLE_ALL_TRACKING(); const rest = REST(p, ['a', 'b']); ENABLE_ALL_TRACKING();

Yes, this one should work. I'd explore how I could eliminate killing tracking, because I expect some use cases without babel (writing directly by hand).

@dai-shi
Copy link
Owner Author

dai-shi commented Mar 20, 2019

$ npx babel-node
> const { proxyState, REST }  = require('./src')
undefined
> const state = { a: { a1: 1, a2: 2 }, b: { b1: 1, b2: 2 }, c: { c1: 1, c2: 2 } }
undefined
> const trapped = proxyState(state)
undefined
> trapped.state.a.a1
1
> trapped.affected
[ '.a', '.a.a1' ]
> const rest = trapped.state[REST](['a'])
undefined
> trapped.affected
[ '.a', '.a.a1' ]
> rest.c.c1
1
> trapped.affected
[ '.a', '.a.a1', '.c', '.c.c1' ]

This is what I expect, and it seems working.

@theKashey
Copy link

I spend so much time fighting spread last year. It's so simple to fight it with babel :(
(not super sure that TS could keep it untranspiled )

@faceyspacey
Copy link

(not super sure that TS could keep it untranspiled )

@theKashey using babel + hypothetical babel plugin to transpile TS would be fine, right?

@dai-shi
Copy link
Owner Author

dai-shi commented Mar 20, 2019

Just a thought: can we assume the transpiled result, and do a hack?
The babel repl transpiles a rest operator into:

var a = state.a,
    rest = _objectWithoutProperties(state, ["a"]);

So, what if we can override the implementation of _objectWithoutProperties or detect the call of _objectWithoutProperties and do something.


Hmm, I don't like this idea very much though. I want a solution a) that works without babel, and b) babel just makes it easy.

@faceyspacey
Copy link

faceyspacey commented Mar 27, 2019

fyi, Respond Framework (the Remixx library specifically) is the perfect place for the babel plugin approach to the rest operator--because we already have one. Here's what we have so far (thanks to @ScriptedAlchemy): https://github.com/faceyspacey/remixx/blob/master/src/babel/index.js

@theKashey is u can make a spec for Zack to implement this, that will really help us.

@theKashey
Copy link

👍 , @dai-shi created code to support it, but I didnt updated library yet.

@dai-shi
Copy link
Owner Author

dai-shi commented Mar 27, 2019

@MrWolfZ 's comment: reduxjs/react-redux#1179 (comment)

there are some cases in which unnecessary renders are triggered due to intermediary values being required to produce the final derived value (e.g. the example @epeli posted above, conditional accesses like from @faceyspacey's comment, or something simple like Object.keys(state.items))

Another simple case (which is rather common) and technically difficult one is the rest operator.

const { a, b, ...rest } = state.items;
const [first, second, ...rest] = state.items;

@theKashey
Copy link

Of course, arrays, so we have to have two different rest operators.

@dai-shi
Copy link
Owner Author

dai-shi commented Mar 27, 2019

Yep.

@faceyspacey
Copy link

@dai-shi you added support for the first one with objects, but not with arrays [first, ...rest] ?

@theKashey
Copy link

the current implementation would convert array to the object 🤷‍♂️

@dai-shi
Copy link
Owner Author

dai-shi commented Mar 27, 2019

I think we should be able to take the same approach for array rest.

object rest:

rest = proxied[SYMBOL_OBJECT_REST](['a', 'b']);

array rest:

rest = proxied[SYMBOL_ARRAY_REST](2);

@theKashey
Copy link

Yep.

@dai-shi
Copy link
Owner Author

dai-shi commented Apr 15, 2019

This issue has been almost only about rest operator. If we need to discuss about spread operator, let's open a new issue.

@dai-shi dai-shi changed the title Discussions about rest/spread operators Discussions about rest operator Apr 15, 2019
@dai-shi
Copy link
Owner Author

dai-shi commented Apr 15, 2019

The remaining is to add a note in README.
Is this correct? @theKashey


Advanced Usage

Rest operator

You can't use rest operator to track state usage properly. There is a workaround. Refer this example.

import React from 'react';
import { useReduxState } from 'reactive-react-redux';
import { proxyObjectRest, proxyArrayRest } from 'proxyequal';

const Person = () => {
  const state = useReduxState();
  // const { firstName, lastName, ...rest } = state; // this is not recommended for performance.
  const [firstName, lastName, rest] = proxyObjectRest(state, ['firstName', 'lastName']); // this is workaround.
  return (
    <div>
      <div>{firstName}{' '}{lastName}</div>
      <PersonDetail person={rest} />
    </div>
  );
};

@theKashey
Copy link

Technically the current implementation of rest is 99% implementation of spread, but I am not sure it is the best way.
Probably a small and specific proxy is better

const spread = {
  ...object1,
  ...object2,
} 
// ->
const spread = proxyObjectSpread(object1, object2);
// ------
cosnt proxyObjectSpread = (...objects) => {
  const assingIndex = (object, x) => Object.keys(object).reduce((acc, key) => {acc[key]=x;return acc;},{});
  // create "shape" objects, with values === used used object id
  const spreadKeyHolder = Object.assign({},...(objects.map(assingIndex));
  return new Proxy(spreadKeyHolder, {
    get(key) {
       return objects[spreadKeyHolder[key]][key]; // redirect read
    }
 })
}

PS: You are 100% right.

@theKashey
Copy link

The remaining is to add a note in README.

You know, now I am thinking about adding d.ts for proxyequal, which is quite important for this case.
But I would prefer you if will re-export necessary APIs via your library, or hide everything behind babel-plugin (@ScriptedAlchemy, where are you?)

@dai-shi
Copy link
Owner Author

dai-shi commented Apr 15, 2019

prefer you if will re-export necessary APIs via your library

That's something I'd like to hear. So, do you mean it's cleaner? Any other benefit of re-exporting?

hide everything behind babel-plugin

I'd like to see how it's done. Depending on it, I'd choose which to recommend.

@theKashey
Copy link

So, do you mean it's cleaner? Any other benefit of re-exporting?

You shall not use 5 different libraries, if they are all "behind" one. Some eslint configurations will not let you import packages you are not explicitly depend on.

@dai-shi
Copy link
Owner Author

dai-shi commented Apr 15, 2019

Got that point. I'm aware of the eslint rule.

@dai-shi
Copy link
Owner Author

dai-shi commented Jun 30, 2019

Current status: useTrackedState doesn't support rest operators any more (since v3). The experimental useTrackedSelectors should still support it (the remaining is re-exporting).

For respond-framework, useTrackedSelectors should work better. Waiting for feedbak.

@dai-shi
Copy link
Owner Author

dai-shi commented Sep 25, 2019

Let me close this because of inactivity.
As of now, this library doesn't explicitly support rest operators. (It works by design with false positives = extra re-renders.)

@dai-shi dai-shi closed this as completed Sep 25, 2019
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

3 participants