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

Fix obscure error message when passing an invalid style value for SSR #11173

Merged
merged 7 commits into from
Oct 10, 2017

Conversation

iamdustan
Copy link
Contributor

@iamdustan iamdustan commented Oct 10, 2017

This may be a regression to add to #11065.

Adding a reduced test case for this issue by opening a PR. When trying to server render an iframe with an invalid style tag the following error is thrown:

~/react/ssr-test/node_modules/react-dom/cjs/react-dom-server.node.development.js:651                                                                                                                            var ownerName = getCurrentOwnerName();
                                                                                                                                                                      ^                                                                                                                                                                                                                                                                                                                                                                                                   TypeError: getCurrentOwnerName is not a function                                                                                                                                                               
at getDeclarationErrorAddendum (~/react/ssr-test/node_modules/react-dom/cjs/react-dom-server.node.development.js:651:21)                                                                                   
at assertValidProps (~/react/ssr-test/node_modules/react-dom/cjs/react-dom-server.node.development.js:675:236)                                                                                             
at ReactDOMServerRenderer.renderDOM (~/react/ssr-test/node_modules/react-dom/cjs/react-dom-server.node.development.js:2922:5)                                                                              
at ReactDOMServerRenderer.render (~/react/ssr-test/node_modules/react-dom/cjs/react-dom-server.node.development.js:2755:23)                                                                                
at ReactDOMServerRenderer.read (~/react/ssr-test/node_modules/react-dom/cjs/react-dom-server.node.development.js:2722:19)                                                                                  
at renderToString (~/react/ssr-test/node_modules/react-dom/cjs/react-dom-server.node.development.js:2980:25)                                                                                               
at Object.<anonymous> (~/react/ssr-test/src/Ssr.js:5:3)                                                                                                                                                    
at Module._compile (module.js:570:32)                                                                                                                                                                      
at loader (~/react/ssr-test/node_modules/babel-register/lib/node.js:144:5)                                                                                                                                 
at Object.require.extensions.(anonymous function) [as .js] (~/react/ssr-test/node_modules/babel-register/lib/node.js:154:7) 

I would expect this to throw the invalid props-style warning of:

The `style` prop expects a mapping from style properties to values, not a string. For example, style={{marginRight: spacing + \'em\'}} when using JSX.

This can be traced back to this line here where we are missing the third argument to assertValidProps:

assertValidProps(tag, props);

This file is not checked by flow which is possibly how the missed argument slipped through, though naively ading the @flow pragma to that file has 39 errors.

What should the fix be? (I included a commit to implement the only solution I’ve considered thus far)

@gaearon
Copy link
Collaborator

gaearon commented Oct 10, 2017

Can you please declare it in the top scope so we don’t need to worry about allocating?

@gaearon
Copy link
Collaborator

gaearon commented Oct 10, 2017

You can just pass emptyFunction.thatReturnsNull.

@gaearon
Copy link
Collaborator

gaearon commented Oct 10, 2017

And yes, we should Flow-ify that file. But that’s a separate problem. Wanna do it in another PR?

@gaearon gaearon mentioned this pull request Oct 10, 2017
19 tasks
@gaearon
Copy link
Collaborator

gaearon commented Oct 10, 2017

Prettier is failing.

1 similar comment
@gaearon
Copy link
Collaborator

gaearon commented Oct 10, 2017

Prettier is failing.

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

Use emptyFunction.thatReturnsNull

@gaearon gaearon changed the title React 16 SSR Iframe failing test case Fix obscure error message when passing an invalid style value for SSR Oct 10, 2017
@gaearon
Copy link
Collaborator

gaearon commented Oct 10, 2017

Filed #11175 for Flow.

@gaearon gaearon merged commit 9b4e4e1 into facebook:master Oct 10, 2017
@iamdustan iamdustan deleted the react-ssr-iframe-error branch October 10, 2017 16:45
@iamdustan
Copy link
Contributor Author

👌 thanks, Dan!

@gaearon
Copy link
Collaborator

gaearon commented Nov 3, 2017

React 16.1.0-beta has been released. Please update react, react-dom, and react-test-renderer (if you use it) to this version and let us know if it solved the issue! We’d appreciate if you could test before Monday when we plan to get 16.1.0 out.

@ValentinFunk
Copy link

Would it be possible for React to throw for invalid styles when inside React.StrictMode?

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

Successfully merging this pull request may close these issues.

4 participants