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

Add redux examples #65

Merged
merged 9 commits into from Feb 22, 2017
Merged

Add redux examples #65

merged 9 commits into from Feb 22, 2017

Conversation

ekosz
Copy link
Contributor

@ekosz ekosz commented Jan 16, 2017

  1. Add example for connected redux application
  2. Add example for universal connected redux application

This is almost done, but I've gotten stuck on an error with the universal version. The server renders the page correctly, but then the front-end errors out with:

Uncaught (in promise) TypeError: Object is not async iterable

It seems to be a problem with getStoreRenderArgs. @taion have you seen an error like that in the past?

@taion
Copy link
Contributor

taion commented Jan 16, 2017

yes... sigh. it's a core-js bug, zloirock/core-js#262 / zloirock/core-js#267.

see babel issue: babel/babel#4783
and discussion of workaround used elsewhere: ctrlplusb/react-universally#182

i don't have a good answer for this right now

@taion
Copy link
Contributor

taion commented Jan 16, 2017

(i mean, the pr i have up on core-js, sorta, but i'm not sure it's correct)

@ekosz
Copy link
Contributor Author

ekosz commented Jan 16, 2017

Ah, cool. Not a problem. When you get a chance can you look over the examples and make sure they're idiomatic for found? Other than than error it should all be working.

@taion
Copy link
Contributor

taion commented Jan 21, 2017

Sorry – I've been a little swamped this week. I haven't forgotten about this PR, I just haven't had a chance to look more closely yet.

@ekosz
Copy link
Contributor Author

ekosz commented Jan 21, 2017

@taion Not a problem at all. I know you're busy, don't worry about.

@taion
Copy link
Contributor

taion commented Jan 25, 2017

babel/babel#5195 would also fix the problem

Copy link
Contributor

@taion taion left a comment

Choose a reason for hiding this comment

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

Sorry it took so long for me to get to these. These look great!

A couple of other comments. Could you:

<body>
<div id="root">${ReactDOMServer.renderToString(element)}</div>
<script>
window.__PRELOADED_STATE__ = ${JSON.stringify(state)};
Copy link
Contributor

Choose a reason for hiding this comment

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

xss vulnerability here

],
},
];

Copy link
Contributor

Choose a reason for hiding this comment

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

extraneous newline?

App.propTypes = propTypes;

export default App;

Copy link
Contributor

Choose a reason for hiding this comment

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

extraneous newline?

@@ -0,0 +1,54 @@
import React from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

do you think it'd make sense to put everything here, for this example specifically? it's how i have it in the others... i think it'd still be short enough to be readable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah not a problem.

@@ -0,0 +1,19 @@
{
"name": "connected-redux",
Copy link
Contributor

Choose a reason for hiding this comment

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

i would just call this "redux"

@@ -0,0 +1,23 @@
{
"name": "universal-connected-redux",
Copy link
Contributor

Choose a reason for hiding this comment

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

i would just call this "universal redux"

@taion
Copy link
Contributor

taion commented Feb 14, 2017

@ekosz BTW the Babel issue got fixed in babel 6.23.

@ekosz
Copy link
Contributor Author

ekosz commented Feb 14, 2017

@taion Awesome. I'll update these examples after work today

@taion
Copy link
Contributor

taion commented Feb 14, 2017

Well, as soon as babel-runtime 6.23 gets published, anyway.

@ekosz ekosz changed the title WIP: Add more examples Add redux examples Feb 14, 2017
@ekosz
Copy link
Contributor Author

ekosz commented Feb 14, 2017

@taion Updated the examples. Unfortunately even after updates all of the dependencies I'm still getting that error. It looks like babel-plugin-transform-runtime@6.23.0 was released, but other modules are still relying on the old plugin?

@taion
Copy link
Contributor

taion commented Feb 14, 2017

babel-runtime 6.23.x actually hasn't been released yet... that's the package that needs the update, sorry

@taion
Copy link
Contributor

taion commented Feb 14, 2017

This looks great though. I'll merge once Babel updates.

@taion
Copy link
Contributor

taion commented Feb 22, 2017

Oops, got sidetracked. Thanks again!

@taion taion merged commit f094b36 into 4Catalyzer:master Feb 22, 2017
@ekosz
Copy link
Contributor Author

ekosz commented Feb 22, 2017

@taion Not a problem. Glad I could give back in a small way.

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.

None yet

2 participants