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

bug(helpers): dataToJS retrieves a new copy each render #84

Closed
Venryx opened this Issue Mar 19, 2017 · 9 comments

Comments

Projects
None yet
2 participants
@Venryx

Venryx commented Mar 19, 2017

Using dataToJS() in the @connect() function breaks the shouldComponentUpdate functionality -- that is to say, every time @connect() calls dataToJS(), it gets a new object returned even if the database contents are the same.

This is an issue as it causes unnecessary re-rendering of the component.

Is this a known issue? Is there a workaround for it?

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Mar 19, 2017

Owner

@Venryx This is something that seems to be due to the usage of immutable.

The plan for v2.0.0 (outlined in the roadmap) is to remove the usage of immutable, which would mean there will be no more need for dataToJS in the connect. Hopefully that will get shouldComponentUpdate to work correctly and not cause unnecessary re-rendering.

Owner

prescottprue commented Mar 19, 2017

@Venryx This is something that seems to be due to the usage of immutable.

The plan for v2.0.0 (outlined in the roadmap) is to remove the usage of immutable, which would mean there will be no more need for dataToJS in the connect. Hopefully that will get shouldComponentUpdate to work correctly and not cause unnecessary re-rendering.

@Venryx

This comment has been minimized.

Show comment
Hide comment
@Venryx

Venryx Mar 19, 2017

Here's a (not that great) workaround, for the moment:

class DBPathInfo {
	lastTimestamp = -1;
	cachedData;
}
let pathInfos = {} as {[path: string]: DBPathInfo};
export function GetData(firebase: FirebaseDatabase, path: string) {
	let info = pathInfos[path] || (pathInfos[path] = new DBPathInfo());
	var timestamp = (firebase as any)._root ? (firebase as any)._root.entries.find(a=>a[0] == "timestamp")[1].get(path) : null;
	if (timestamp && timestamp != info.lastTimestamp) {
		info.lastTimestamp = timestamp;
		info.cachedData = helpers.dataToJS(firebase, path);
	}
	return info.cachedData;
}

As can be seen, it checks if the data at a position is new each time, based on the timestamp.

If so, it transforms that raw data using the dataToJS transformer, then returns it.

If not, it returns the cached result of that operation from last time.

The main concern atm is that I'm not sure it'll work for "embedded" data, ie, data that was retrieved from an ancestor node-request, and therefore has no "timestamp" property itself. I suppose in that case, I could have it search for ancestor timestamps as well, but that I'm not sure if the timestamps for node-requests at multiple depths are kept in-sync.

Venryx commented Mar 19, 2017

Here's a (not that great) workaround, for the moment:

class DBPathInfo {
	lastTimestamp = -1;
	cachedData;
}
let pathInfos = {} as {[path: string]: DBPathInfo};
export function GetData(firebase: FirebaseDatabase, path: string) {
	let info = pathInfos[path] || (pathInfos[path] = new DBPathInfo());
	var timestamp = (firebase as any)._root ? (firebase as any)._root.entries.find(a=>a[0] == "timestamp")[1].get(path) : null;
	if (timestamp && timestamp != info.lastTimestamp) {
		info.lastTimestamp = timestamp;
		info.cachedData = helpers.dataToJS(firebase, path);
	}
	return info.cachedData;
}

As can be seen, it checks if the data at a position is new each time, based on the timestamp.

If so, it transforms that raw data using the dataToJS transformer, then returns it.

If not, it returns the cached result of that operation from last time.

The main concern atm is that I'm not sure it'll work for "embedded" data, ie, data that was retrieved from an ancestor node-request, and therefore has no "timestamp" property itself. I suppose in that case, I could have it search for ancestor timestamps as well, but that I'm not sure if the timestamps for node-requests at multiple depths are kept in-sync.

@Venryx

This comment has been minimized.

Show comment
Hide comment
@Venryx

Venryx Mar 19, 2017

@prescottprue Ah, sounds good. : )

Thankfully, the performance impact is not yet enough in my app to be a problem. Perhaps v2 will be ready by the time it is needed. (I'm pretty sure it would be a problem eventually)

Venryx commented Mar 19, 2017

@prescottprue Ah, sounds good. : )

Thankfully, the performance impact is not yet enough in my app to be a problem. Perhaps v2 will be ready by the time it is needed. (I'm pretty sure it would be a problem eventually)

@prescottprue prescottprue added bug enhancement v2.0.0 and removed bug labels Mar 19, 2017

@prescottprue prescottprue self-assigned this Mar 19, 2017

prescottprue added a commit that referenced this issue Mar 21, 2017

chore (docs): data flow diagram and roadmap updates (#86)
* Data flow diagram added with link in FAQ section of README
* Roadmap updated with recent issues (including #72, #82, #84, #85)
* Contributing updated with link to roadmap
@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue
Owner

prescottprue commented Mar 21, 2017

Roadmap updated

@prescottprue prescottprue added on-roadmap and removed enhancement labels Apr 5, 2017

@prescottprue prescottprue added this to the v2.0.0 milestone May 5, 2017

@prescottprue prescottprue removed the v2.0.0 label May 5, 2017

@Venryx

This comment has been minimized.

Show comment
Hide comment
@Venryx

Venryx May 9, 2017

The Roadmap says "Progress: v2.0.0-alpha has been started, view the branch", however the link gives "page not found".

Has work started on it, or was it just a placeholder for soon-to-be-done work?

And if work has started, how far is it? (if it's far enough, I might prefer to try/contribute-to it even if it's partially broken, as I'm currently doing an optimization phase in my project, and this is somewhat of a blocker for an optimization I have planned)

In the meantime, I'll see if there's a way I can "resolve" the ImmutableJS maps to normal objects manually. (perhaps in a middleware)

Venryx commented May 9, 2017

The Roadmap says "Progress: v2.0.0-alpha has been started, view the branch", however the link gives "page not found".

Has work started on it, or was it just a placeholder for soon-to-be-done work?

And if work has started, how far is it? (if it's far enough, I might prefer to try/contribute-to it even if it's partially broken, as I'm currently doing an optimization phase in my project, and this is somewhat of a blocker for an optimization I have planned)

In the meantime, I'll see if there's a way I can "resolve" the ImmutableJS maps to normal objects manually. (perhaps in a middleware)

@Venryx

This comment has been minimized.

Show comment
Hide comment
@Venryx

Venryx May 9, 2017

I managed to update the library myself to use plain JS objects.

Granted, I implemented the change as a "hack" rather than an organic reworking, but for people just waiting for the next version, it seems to work fine.

The changes I made are as follows.

"node_modules/react-redux-firebase/dist/reducer.js":

-var initialState = (0, _immutable.fromJS)(emptyState);
+var initialState = emptyState;
+
+var u = require("updeep");
+initialState.getIn = function(pathNodes, notSetValue) {
+	let result = this;
+	for (let pathNode of pathNodes) {
+		if (result == null) break;
+		result = result[pathNode];
+	}
+	if (result == null)
+		return notSetValue;
+	return result;
+};
+initialState.setIn = function(pathNodes, value) {
+	return u.updateIn(pathNodes.join("."), u.constant(value), this);
+};
+initialState.deleteIn = function(pathNodes) {
+	//return u.updateIn(pathNodes.join("."), undefined, this);
+	return u.updateIn(pathNodes.slice(0, -1).join("."), u.omit(pathNodes.slice(-1)[0]), this);
+};
+
+_immutable.fromJS = a=>a;

I also had to run npm install --save updeep in my project, since the code above depends on that package for updating deep keys/data -- the repo for it is here: https://github.com/substantial/updeep

Venryx commented May 9, 2017

I managed to update the library myself to use plain JS objects.

Granted, I implemented the change as a "hack" rather than an organic reworking, but for people just waiting for the next version, it seems to work fine.

The changes I made are as follows.

"node_modules/react-redux-firebase/dist/reducer.js":

-var initialState = (0, _immutable.fromJS)(emptyState);
+var initialState = emptyState;
+
+var u = require("updeep");
+initialState.getIn = function(pathNodes, notSetValue) {
+	let result = this;
+	for (let pathNode of pathNodes) {
+		if (result == null) break;
+		result = result[pathNode];
+	}
+	if (result == null)
+		return notSetValue;
+	return result;
+};
+initialState.setIn = function(pathNodes, value) {
+	return u.updateIn(pathNodes.join("."), u.constant(value), this);
+};
+initialState.deleteIn = function(pathNodes) {
+	//return u.updateIn(pathNodes.join("."), undefined, this);
+	return u.updateIn(pathNodes.slice(0, -1).join("."), u.omit(pathNodes.slice(-1)[0]), this);
+};
+
+_immutable.fromJS = a=>a;

I also had to run npm install --save updeep in my project, since the code above depends on that package for updating deep keys/data -- the repo for it is here: https://github.com/substantial/updeep

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue May 9, 2017

Owner

Nice @Venryx! Will check into this a bit more. I believe I was able to get something similar working without updeep, but this is great for comparison.

Owner

prescottprue commented May 9, 2017

Nice @Venryx! Will check into this a bit more. I believe I was able to get something similar working without updeep, but this is great for comparison.

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Jul 3, 2017

Owner

Would be interested to see if v2.0.0-alpha.* versions solve this for you since ImmutableJS is not used, which means no use of dataToJS.

v2.0.0-beta is going to be out very soon, and should also solve it. If that is not the case, I would like to get to the bottom of it before bringing v2.0.0 out of pre-release.

Owner

prescottprue commented Jul 3, 2017

Would be interested to see if v2.0.0-alpha.* versions solve this for you since ImmutableJS is not used, which means no use of dataToJS.

v2.0.0-beta is going to be out very soon, and should also solve it. If that is not the case, I would like to get to the bottom of it before bringing v2.0.0 out of pre-release.

@prescottprue prescottprue changed the title from DataToJS retrieves a new copy each render to bug(helpers): dataToJS retrieves a new copy each render Jul 4, 2017

@prescottprue prescottprue modified the milestones: v1.5.0, v2.0.0 Sep 2, 2017

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Sep 2, 2017

Owner

v2 has seen major adoption, so this thread may be dead, but those still working with v1:

dataToJS does not need to be used in connect! Plenty go without it all together

Since data is stored in an Immutable Map in v1, you can just pass the Map object down as props in the connect function. That way JS object conversion (most often through .toJS() or dataToJS) can happen in other places such as Component methods and the render function.

This can be particularly useful if you want to use any of the handy Immutable methods within your code (which are removed when converting it to a JS object).

In the near future there will be a short section about this in the v1 docs. Closing since this can be worked around with v1, and was intentionally designed to not happen for v2.

Owner

prescottprue commented Sep 2, 2017

v2 has seen major adoption, so this thread may be dead, but those still working with v1:

dataToJS does not need to be used in connect! Plenty go without it all together

Since data is stored in an Immutable Map in v1, you can just pass the Map object down as props in the connect function. That way JS object conversion (most often through .toJS() or dataToJS) can happen in other places such as Component methods and the render function.

This can be particularly useful if you want to use any of the handy Immutable methods within your code (which are removed when converting it to a JS object).

In the near future there will be a short section about this in the v1 docs. Closing since this can be worked around with v1, and was intentionally designed to not happen for v2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment