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

implement untrack API #24

Merged
merged 9 commits into from
Oct 12, 2019
Merged

implement untrack API #24

merged 9 commits into from
Oct 12, 2019

Conversation

dai-shi
Copy link
Owner

@dai-shi dai-shi commented Oct 10, 2019

closes #23.

  • rename function
  • add test
  • add example
  • update docs

@dai-shi
Copy link
Owner Author

dai-shi commented Oct 10, 2019

I'm thinking of renaming the function. It may sound too general.
I would expect we wouldn't need this API for most (or just some) use cases.
And, if we create a custom hook, it won't shown in a render function.

So, let's make it more descriptive.
Maybe, getUntrackedObject, or getUntracked.

@dai-shi
Copy link
Owner Author

dai-shi commented Oct 10, 2019

untrack sounds like stopping tracking, which is not what it does.

@4nte
Copy link

4nte commented Oct 11, 2019

getUntrackedObject is verbose and most descriptive of all, which may help users to understand it easier. +1

IMO tracking is an implementation detail of the library and therefore any name we give to this function will seem odd / out of place to a user as they will feel that they're doing some extra work for the library. I understand it's a limitation, for now, hopefully, there's a better solution for the future.

@dai-shi
Copy link
Owner Author

dai-shi commented Oct 11, 2019

Yeah, this function and the other one trackMemo is exposing implementation details. We would probably build utility functions or middleware based on them in the future. These are primitives. It's a good start.

src/index.d.ts Outdated
@@ -17,4 +17,4 @@ export const createContainer: <State, Update, Props>(
// deep proxy utils

export const trackMemo: (obj: unknown) => void;
export const getUntracked: <T>(obj: T) => T;
export const getUntrackedObject: <T>(obj: T) => T;
Copy link

@4nte 4nte Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since the use of getUntrackedObject is limited to objects only the type definition can be improved to:

export const getUntrackedObject: <T extends object>(obj: T) => T;

Unfortunately, array is a typeof object so it will not throw an error but at least it will exclude the primitive types.

See http://www.typescriptlang.org/play/#code/GYVwdgxgLglg9mABAcwKZQKpigJwIYQDWqAJgPIBGAVqtADwAqiqAHlKmCQM6JzW1QAfAAo+VAFwMAlJMQBvAFCJliHOhA4kYgNwKAvgoVpM2fEVKUa0YXMTA4ccYgCMiPVN3GsuAsXL9rAG1nAF0PRAB6COVxYUMvU18LAKhhZw8jdG8zP0sBYQAiezgCjISfc38rVPASVGAYMFIyrMTKvOswEAAbbpaTCtyU4VwQVA8gA

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since the use of getUntrackedObject is limited to objects

Uh, no, I would like to accept any value. That would make developing custom dispatch easier.
It also accepts arrays.

OK, that's a good sign for adding comments. Will do.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you suggest to change the function name if it accepts non-object values?

Copy link

@4nte 4nte Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be for the better to leave it as getUntrackedObject since it's intended
to be used for objects - might reduce misusage of it.

Copy link

@4nte 4nte Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, no, I would like to accept any value. That would make developing custom dispatch easier.

How about 2 functions are exported?
getUntrackedObject should do exactly what it does -- attempt to untrack an object.
getUntracked untrack any value, not limited to objects.

We can refactor getUntrackedObject to omit the if(isPlainObj(obj)) check and to assume that given parameter is an object,

And introduce another API function getUntracked which is safe to call with any value. It's implementation should do the isPlainObj(obj) check and call the getUntrackedObject.

This way, getUntrackedObject will have less overhead for direct users.

Edit:
Or naming them like this?

export const untrack = <T extends object>(obj: T): T = {}
export const getUntrackedObject = <T>(obj: T): T = {}

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for you suggestion. But, I wouldn't take that approach because of various reasons.

  1. I would prefer to export less functions as possible. getUntrackedObject is much safer and wouldn't be misused.
  2. I would like to hide implementation details. isPlainObject is actually an implementation detail. I could imagine that would be changed in the future. So, I don't want users of this library to assume what is an object that would be tracked.
  3. As far as I understand, the current implementation of isPlainObject is pretty lightweight, and it wouldn't be a bottleneck. If, in the future, we find use cases that this would impact performance, we can consider exporting another function for optimization.

I hope those make sense.

Copy link
Owner Author

@dai-shi dai-shi Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would make developing custom dispatch easier.

I might be wrong about this. I will try it in an example.

This is what I've got.

const untrackDeep = <T>(obj: T) => {
if (typeof obj !== 'object' || obj === null) return obj;
const untrackedObj = getUntrackedObject(obj);
if (untrackedObj !== obj) return untrackedObj;
const newObj = {} as T;
let modified = false;
Object.entries(obj).forEach(([k, v]) => {
const vv = untrackDeep(v);
newObj[k as keyof T] = vv;
modified = modified || v !== vv;
});
return modified ? newObj : obj;
};

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, it can be easier for getUntrackedObject to return null if obj is not a proxy.

@dai-shi
Copy link
Owner Author

dai-shi commented Oct 12, 2019

Merging this. Thanks for the feedback.

@dai-shi dai-shi merged commit 2eccd11 into master Oct 12, 2019
@dai-shi dai-shi deleted the untrack-api branch October 12, 2019 07:25
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

Successfully merging this pull request may close these issues.

A new API for reverting proxy
2 participants