Rename affected to accessed

[stephenh]

Up-front disclaimer this: a) is about proxy-compare and b) would admittedly be a large breaking change, but FWIW another "tripped up the newbie" gotcha for me was the naming of "affected" instead of "accessed".

Granted, I get it now, but I was doing things like:

  useEffect(() => {
    pathList.current = affectedToPathList(state, affected, true);
    // my debug code while poking around to learn
    console.log("CHANGED", pathList.current);
  });

And then wondering "...why did this property change? It didn't change..."

Because "affected" as a term typically means "past tense" / "has already been effected" / "changed".

Granted, I get it now, as mentioned several times in the proxy-compare docs/readme, what we're really recording and reporting is accessed properties.

So, again feel free to just say "nope, too late", but fwiw I'll volunteer to at least submit proxy-compare and valtio PRs to rename affected --> accessed if you're game. :-)

Pretty awesome that proxy-compare is used but ~4-5 other frameworks, which is great, but also understand that raises the bar for breaking changes.

Thanks!

[dai-shi]

affected in the code refers to a WeakMap, so even if we change it to accessed it's still confusing. What do you think?
But, I'm fine to change the name for docs and API surface (do we use it in API? maybe it's shown in React DevTools.)

Pretty awesome that proxy-compare is used but ~4-5 other frameworks, which is great, but also understand that raises the bar for breaking changes.

well, if you can help me, we can do. I don't think it's so breaking. Ah, affectedToPathList is exported, so it is breaking.

[stephenh]

affected in the code refers to a WeakMap, so even if we change it to accessed it's still confusing. What do you think?

Hm, yeah it's a WeakMap, but it's a WeakMap whose entries are added "on access" / on usage, so I think that's fine/seems intuitive?

Ah, affectedToPathList is exported, so it is breaking.

Yeah; we could also export both affectedToPathList and accessedToPathList and just deprecate the old one for a few releases.

[dai-shi]

On second thought, it's mostly in the source code, so code comment would just help?

I just feel like changing the name of WeakMap is suboptimal anyway.

type Affected = WeakMap<object, Used>;
type Accessed = WeakMap<object, Used>;

affectedToPathList is just an internal API for libraries. Not to be used by end developers.

[stephenh]

so code comment would just help

Yeah, I suppose... Agreed it is mostly internal, but even in the readme of proxy-compare I think it's clearer to use "accessed".

https://github.com/dai-shi/proxy-compare/pull/52/files

Because like this example:

const d = { e: "3" };
const original = { a: "1", c: "2", d };
const accessed = new WeakMap();
const proxy = createProxy(original, accessed);

proxy.a // Will mark as used and track its value.
// This will update the accessed WeakMap to be
// { original: Set("a") }

The map is populated when a is accessed, not when it is changed/affected.

Also, when calling isChanged, the current example variable names look like:

proxy.a
isChanged(obj, { a: "1" }, affected)

But even though a is in the affected map, and we're passing "affected" to isChanged, we don't really know if every entry in affected is actually affected or not. It's only after isChanged compares the 1st arg & 2nd arg, with the subset of accessed properties, that we can really know how many accessed properties are actually affected.

As a thought experiment, if the API for isChanged was not just "return boolean", but "return the list of actually affected fields", then it would look something like:

const accessed = new WeakMap()
...
proxy.a
const affected = isChanged(s1, s2, accessed);

Granted, I'm not actually suggesting changing the return value of isChanged, I'm just highlighting that I think what is currently called affected is not actually accessed. :-)

But also agreed, it is mostly an internal / for devs only, so this isn't going to trip up regular users of valtio, only maybe the power users who peak under the hood. :-) So 🤷 still seems worth changing imo, but happy to defer to you.

[stephenh]

I suppose I am really interpreting the term as "this key was affected by a mutation", but perhaps you're thinking of it as "this key was affected by a proxy access".

[dai-shi]

I suppose I am really interpreting the term as "this key was affected by a mutation", but perhaps you're thinking of it as "this key was affected by a proxy access".

Exactly! That explains.