-
Notifications
You must be signed in to change notification settings - Fork 23
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
server-side rendering and sagas issue (originally "React Performance Measurement Error") #21
Comments
Nevermind, i got this to work (i missed "model: " in combineReducer but it throws this now:
Edit: Actually it didn't resolve the first errors. The URLSearchParameters error happens in my console, the other in the browser. I tried to rewrite the purpose of the function in ApiClient.js to get rid of it with this: _createClass(ApiClient, [{
key: 'queryString',
value: function queryString(params) {
var str = [];
var s = '';
for(var p in params) {
if (params.hasOwnProperty(p)) {
str.push(encodeURIComponent(p) + "=" + encodeURIComponent(params[p]));
}
}
var s = str.join("&");
return s ? '?' + s : '';
}
}]); This resolved the error but now this happens:
I guess I'm doing something very very very wrong but I absolutely cannot figure it out. |
Hi! thanks for reporting this issue! I did test the default ApiClient, but only minimally. So chances are this is a stupid issue that can be resolved easily. For now, could you try adding the sample ApiClient from https://github.com/uniqueway/redux-crud-store/blob/feature/api-client/docs/Sample-Api-Client-with-Superagent.md to your codebase, and then running I'll try to set up a better testing environment for the latest master release and make sure it all works. As for the React performance measurement code, is this a feature of a more recent version of react? Or do you think this is coming from a library you've included? Send me any clues that will help me track this down |
@mxmtsk could you take a look at facebook/react#6949 and see if it's related to your problem at all? I haven't used react perf at all, but it looks related. It might help me better understand where the issue comes from if you look into this for me. thanks!! |
Ah ha. looks like I messed up by including URLSearchParams in this library because of browser support: https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams. IE and Safari don't have it. I've got a commit (c6804d8) fixing that ready for testing alongside the react-perf stuff |
@mxmtsk could you send me the rough outline of the format of the data that comes from the server? Currently a big limitation of this library is that it expects your response to have a "data envelope" like this:
for every response. I'm aware that's awful, and I want to set up normalizr integration (see #19) so that this is customizable. If you are able to implement your own ApiClient, you should be able to customize what it returns to provide something in that format. I could also consider a temporary solution of checking the response for a data key and if it doesn't include one, wrapping it in a data envelope. Does that make sense to you? Are you able to test if that solution works? If it does, I could add this to the library as a workaround until #19 is implemented. |
Hey, Indeed, I do not have a data wrapper around my response, I added it for testing purposes and it will process data, but the initial error will not go away. As i understood the discussion correctly in the react repo it seems that the performance error get's thrown by the following error (getIn...). This error changed from I think this is an immutable problem. When I added |
so in mapStateToProps you're doing something like this, right?
What does console.log(state.models) return? Is it an Immutable map? I can't think of a reason why it wouldn't be an immutable map, unless you're doing something I haven't seen before. Could you post your package.json? (or at least the dependencies part) |
Ah, I reviewed your code more closely now. I haven't used the select function extensively, it's an addition by @hallettj . Maybe he has some insight. In the meantime, what about changing your mapStateToProps code to this to see what happens:
UPDATE: I've reviewed select a bit more thoroughly, and it does work like I expected, so probably this won't help. Ah ha, you've posted below - it didn't help. |
It returns Object {cities: Object} I am using xkawi/react-universal-saga as a boilerplate. My deps don't differ right now from the boilerplate as I'm just playing around. Changing it to selectCollection does not change anything, same errors |
I'll clone that repo and see if I can duplicate the error |
OK I've duplicated your boilerplate and gotten the error. I'm still not sure why it happens, but a workaround is to import fromJS from immutable and change mapStateToProps:
You'll also need to change the line that says I'm going to keep digging to figure out why it isn't being stored as immutable in the first place. My one hunch right now is the server side rendering UPDATE: currently my issue is that the FETCH is dispatched by the server, and then no FETCH_SUCCESS or FETCH_ERROR comes back. It must be a saga issue. Let me know if you need my code to get this far |
I managed to get to the point that I don't get an error back but cities.data is empty when I console.log it in my component render. is that what you experience too? |
Yes. I think the problem is with the server side rendering. I don't know how you set up your sagas, but because this boilerplate has its own sagas, you need to fork the two sagas. My current code does the fork on the client side, but that means it's important to NOT dispatch fetch on the server side. It's probably important that async actions should only be dispatched on the client side. I don't have any experience with server side rendering, but hopefully there's a way to trigger the action on the client side render. I wonder how that's accomplished normally... |
Server-Side-Rendering is what get's me frustrated all the time too... at least we're sitting in the same boat :D In the boilerplate on this line there is an example with preload. I think this is for SSR. It get's called in the server.js. I tried to apply the crudSaga to preload but that didn't work... |
OK I can't spend more time on this today. I think I've almost got it, but I have an issue with babel-polyfill being missing that throws If you are able to find the solution and propose a fix here that would help smooth the process for users in the future, I will be eternally grateful |
I think my conclusion is that the issue is that you can't do AJAX in the server code. I think it's possible for this library to handle this + SSR, but for now, it's out of scope, so I'm going to close this issue. I think https://www.npmjs.com/package/redux-async-connect will be able to help you if you want to solve this problem. I'd love to hear back from you if a) you think there is a different reason for this that I can solve in this library or b) you figure out how to solve it and want to share the solution. Thanks for putting the code through the ropes! It's much appreciated! |
Hi,
I've tried to implement redux-crud-store following the ReadMe file. As soon as I call my route in the browers I'll get this messages:
I can't get my head around where the error lies but it does get fired when I call fetchCollection
Landingpage.js
modules/cities.js
The text was updated successfully, but these errors were encountered: