Skip to content
This repository was archived by the owner on Jun 4, 2024. It is now read-only.

Fix props generation. #84

Merged
merged 3 commits into from
Dec 17, 2018
Merged

Fix props generation. #84

merged 3 commits into from
Dec 17, 2018

Conversation

T4rk1n
Copy link
Contributor

@T4rk1n T4rk1n commented Dec 12, 2018

  • n_clicks/n_clicks_timestamp had a wrong PropType integer changed to number.
  • omit n_clicks/n_clicks_timestamp from wrapped element props.

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Dec 12, 2018

@rmarren1 Please review

@rmarren1
Copy link
Contributor

One possible problem:
React 16 will render props in the HTML source, so

{<div n_clicks={0}></div>} -> <div n_clicks=0></div>

as opposed to React 15 which does not do this

{<div n_clicks={0}></div>} -> <div></div>

basically this: https://reactjs.org/blog/2017/09/08/dom-attributes-in-react-16.html

If somebody is building a Dash app using React 16 and has something dependent on this rendering functionality (e.g. a Selenium test which clicks a button and makes sure the n_clicks prop is correct, which is a reasonable thing to do), this would break their tests.

There may be 0 such cases, but it is possible. Does this change have any functional impact? I'd prefer to wait until 1.0 to be careful if it does not.

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Dec 13, 2018

Right now dash app only runs on react 15 so nobody has anything that depends on this, we tests those using output to children. I also don't think it's a good idea to write those props to the DOM, they are included in all html props with a default and the write operation on the DOM still has a performance overhead.

@rmarren1
Copy link
Contributor

You can use react 16 by setting the dash version on the renderer here

An example app:

import dash_renderer
dash_renderer._set_react_version('16.2.0')

import dash
import dash_html_components as html

app = dash.Dash(__name__)
app.layout = html.Div('hello', n_clicks=0, n_clicks_timestamp=100)

app.run_server(debug=True, port=8888)

The DOM contains those props now:
image

We don't use test using these, but it is possible that somebody could be testing this way using a webdriver, and this change would break. I'm thinking of something similar to this test where a DOM element is taken by id then compared to a pre-defined string.

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Dec 13, 2018

That is only valid for testing and there are other way to test it. I mean those props will be written everywhere, even for html element that are not clickable so it doesn't make sense to write them to the DOM for all of them.

@Marc-Andre-Rivet
Copy link
Contributor

@rmarren1 @T4rk1n If someone wrote a test dependent on the presence of the prop in the react data structure accessible from the DOM, this makes their test dependent on internal implementation details of React, and it does not make much sense for us to support a hack test case.

This test can easily be done by either hooking up to a Dash callback or by adding a spy on 'click' and checking the count.

Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet left a comment

Choose a reason for hiding this comment

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

💃

@rmarren1
Copy link
Contributor

rmarren1 commented Dec 13, 2018

That makes sense, was being a bit pedantic 💃
(https://xkcd.com/1172/)

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

Successfully merging this pull request may close these issues.

3 participants