[Core] Add uniqueId generator #4253

ajihyf · 2016-05-13T01:40:00Z

PR has tests / docs demo, and is linted.
Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

oliviertassinari · 2016-05-13T19:17:47Z

src/utils/getUniqueIdGenerator.js

+function getUniqueIdGenerator() {
+  let index = 0;
+  return () => {
+    return `mui-id-${index++}`;


I don't think that it would work. Have a look at #3533.

TL;DR The index increase faster on the server-side than on the client-side.

This function returns a generator function instead of a uniqueId directly.

@oliviertassinari This id generator is created per context, i.e. per request. If the client and server call the getUniqueId function in the same order then this method is deterministic. And since every request will have it's own context no two requests will have reference to the same index variable captured in that function.

Oh sorry, I have overlooked the context part 👍. I though we were going back to the previous implementation.

oliviertassinari · 2016-05-14T09:04:25Z

src/utils/getUniqueIdGenerator.spec.js

+
+  describe('one generator', () => {
+    it('should generate different uniqueIds each time it is invoked', () => {
+      assert.notEqual(gen(), gen());


I would rather use notStrictEqual.

@oliviertassinari The generated ids are used as html tag attributes in most cases, which may contain an implicit type conversion.

const id1 = 1; const id2 = '1'; document.head.id = id1; document.body.id = id2; console.log(id1 === id2); // false console.log(document.head.id === document.body.id); // true

id1 and id2 will pass notStrictEqual. But document.head and document.body will have conflicts using same id.

Should we call toString and return a string to prevent this implicit type conversion?

@oliviertassinari I agree with your previous opinion. 😲Using notEqual or toString here focus too much on the the generator's use cases, not its own functionality. I should add some tests to TextField.

oliviertassinari · 2016-05-14T09:38:15Z

@AJIH Could you have a look at my comment, squash down the commit and use the PR title as the commit messages? We will be good to go 🚀.

ajihyf · 2016-05-14T12:30:43Z

@oliviertassinari Done! 😁

mbrookes · 2016-05-16T20:41:10Z

I'm guessing the name was chosen to be in keeping with getMuiTheme, but could the module just be called uniqueIdGenerator instead of getUniqueIdGenerator?

ajihyf · 2016-05-17T02:53:58Z

@mbrookes It may be ambiguous to name the module uniqueIdGenerator. The module exports a function of type () => () => String, not () => String. And MuiThemeProvider calls the exported function to get a real uniqueIdGenerator of type () => String as its context.

alitaheri · 2016-05-17T05:47:00Z

@mbrookes it's what it does, hence the name. I think it's more clear this way. it dose give you a new uniqueIdGenerator whenever you call it, it's a factory. 🏭 🏭

mbrookes · 2016-05-17T13:52:22Z

Got it, thanks.

mbrookes · 2016-05-17T14:25:25Z

Docs site doesn't use MuiThemeProvider, so:

warning.js:44 Warning: Failed Context Types: Required context `uniqueIdGen` was not specified in `TextField`. Check the render method of `SelectField`
TextField.js:223 Uncaught TypeError: this.context.uniqueIdGen is not a function

alitaheri · 2016-05-17T18:26:53Z

😱 😱 That's not good. should we put it on the muiTheme or should we patch docs? a context for one single component kinda seems overkill. maybe putting it on muiTheme some how isn't all that bad? But this is the best approach for the issue though. we have to either patch docs or put it on muiTheme.

nathanmarks · 2016-05-17T18:40:27Z

@alitaheri putting it on muiTheme seems like mixing concerns. 😟

alitaheri · 2016-05-17T19:32:07Z

Should we just rename muiTheme to everythingMuiNeedsToOperateSuchAsTheme_UniqueIdGenerator_StyleTransformation_UserAgentAndMore? 😆, We won't have conflicts with other though, which is nice 😆 😆

ajihyf · 2016-05-18T04:48:30Z

😲Doc site has a Master component to provide context to its children. Should we add uniqueIdGen to its context to solve the problem?

ajihyf · 2016-05-18T05:24:55Z

@alitaheri 😱I found another bug of current implementation.

Example code(https://jsfiddle.net/aji_h/6o63e7gL/4/):

class MuiThemeProvider extends React.Component {
  ...
  getChildContext() {
    return {
      muiTheme: this.props.muiTheme || getMuiTheme(),
      uniqueIdGen: this.props.uniqueIdGen || getUniqueIdGenerator(),
    };
  }
}

class Container extends React.Component {
  constructor(props) {
    super(props);
    this.state = {clicked: false};
  }
  onClick = () => {
    this.setState({
      clicked: !this.state.clicked
    });
  };
  render() {
    return (      
      <div>
        <button onClick={this.onClick}>Click Me</button>
        <MuiThemeProvider>{
            this.state.clicked ? 
          <div><TextField /><TextField /></div>
          : <div><TextField /></div>
        }</MuiThemeProvider>
      </div>      
    );
  }
}

After clicking the button, the first TextField keeps its uniqueId of mui-id-0 because it's already mounted. But the second TextField will request a context which contains a new uniqueIdGen, call componentWillMount and get its uniqueId of mui-id-0.

ajihyf · 2016-05-18T06:15:06Z

I have refactored the code to fix the bug above mentioned and added a uniqueIdGen to Master component.😂

alitaheri · 2016-05-18T07:06:45Z

Wow this is nice 👍 I think it's ready, @callemall/material-ui Take a look please 😁

ajihyf · 2016-05-18T12:39:00Z

@alitaheri My pleasure.
@mbrookes Extracted "Using context" section to a single page.😋

mbrookes · 2016-05-18T19:48:22Z

docs/src/app/components/pages/customization/using-context.md

+```
+
+Read more details about uniqueId in the [issue](https://github.com/callemall/material-ui/issues/3757) and the [PR](https://github.com/callemall/material-ui/pull/4253).
+


Not sure the PR thread adds much value - it's mostly comments like this one. 😄

mbrookes · 2016-05-18T19:53:16Z

Some comments on using-context.md above. Also, in the Nav menu, 'Using context' should probably follow Themes. The logical order might be:

Colors
Inline Styles
Themes
Using Context

Simplest to most advanced, and coincidentally alphabetical order. 😄

oliviertassinari · 2016-05-22T10:12:20Z

src/styles/MuiThemeProvider.js

+  };
+
+  state = {
+    uniqueIdGen: this.props.uniqueIdGen || getUniqueIdGenerator(),


Any reason for using the state here? Why not following the muiTheme pattern?
According to the React docs site:

The getChildContext function will be called when the state or props changes.

I see one potential issue, the uniqueIdGen context won't be updated according to the property.

@oliviertassinari I tried using the muiTheme pattern and found a bug described in #4253 (comment)

That looks like the expected behavior. What about this version?

In your version, components in MuiThemeProvider will invoke all lifecycle methods(including componentWillMount, where TextField get its uniqueId) each time the button is clicked because the virtual DOM trees are totally different.

@oliviertassinari You mean MuiThemeProvider should return a new uniqueIdGen whenever its getChildContext is called? I think it should keep uniqueIdGen all its lifecycle.

Yes indeed. getChildContext is called at each rerender. So your point is that getUniqueIdGenerator() should only be called at the mount time. That's sounds good 👍.
But, shouldn't we update the uniqueIdGen state during the componentWillReceiveProps hook?

Updating the uniqueIdGen may cause some issues. Mounted components will keep their ids mui-id-0, mui-id-1...But if there are components will mount, their uniqueIdGen may start at 0, and their ids would be mui-id-0, mui-id-1...😰

I see, let's not update it in this case. It looks like we have reached the limitation of this solution.
I can't see real use cases for this edge case, I think that we are fine.
Should we add a comment?

P.S. I start to feel like using an heuristic for the id generation would have been simpler. 😁

Yeah.😂 In fact, the feature has been discussed a bit before in facebook/react#1137 and facebook/react#4000. We use many hacky methods in this solution because we need a global counter which should reset itself each time React calls render method. But Material-ui is a component library and should not inject the render process.
If MuiThemeProvider is the root provider, many potential issues would be eliminated. But in doc it's only preferable, not necessary.

@oliviertassinari How about using a global uniqueIdGen(like your previous implementation) and adding an API to allow manually reset uniqueIdGenerator before rendering?

oliviertassinari · 2016-06-03T22:28:57Z

The more I think about this PR the more I think we are over-engineering the solution.
Is the solution should really make us depend on the context?
Would depending on the id property be enough (as I do in my closed PR)?

ajihyf · 2016-06-04T02:35:31Z

@oliviertassinari Manually setting id property is enough. 😁I think your PR is better.

oliviertassinari · 2016-06-04T20:57:13Z

Finding an id for every used TextField in an app only to support SSR seems overkill to me.

@alitaheri Do you still feel that way? Both solutions came with their tradeoffs, but I think that the id one is simpler.

alitaheri · 2016-06-05T05:19:11Z

@oliviertassinari I kinda feel the same. In my opinion both solutions seem a bit hacky.

I'd say we should think outside the box a little. This id is used for 2 purposes: 1. aria 2. focus

If we drop the mandatory aria support ( users should provide id of their own to enable this ) and solve the focus problem in a different way, then this won't be a problem.

In other words, the only reason we need this is the focus. How about handling this with js? handle the touchTap on label and call focus on textarea? That should solve it right?

nathanmarks · 2016-06-14T12:57:18Z

@oliviertassinari

This PR never sat super well with me but everyone else seemed happy so I let sleeping dogs lay. It did seem like we were over-engineering and putting a very specific function on context for something which can be (and usually is) handled in user land.

Do you think we should re-use your last PR?

oliviertassinari · 2016-06-16T20:03:23Z

@nathanmarks I like the idea proposed by @alitaheri. We could be using the onTouchTap event for achieving the same behavior. That would allow us to avoid this unique id issue.
Actually, as far as I know, this was the original solution implemented 1 year ago.

nathanmarks · 2016-06-16T22:12:49Z

@oliviertassinari I would hold on implementing anything dependent on onTouchTap

oliviertassinari · 2016-10-04T19:54:40Z

@AJIH I'm closing this issue based on this comment. Thanks!

ajihyf changed the title ~~[Core] Fix uniqueId generating~~ [Core] Add uniqueId generating May 13, 2016

ajihyf changed the title ~~[Core] Add uniqueId generating~~ [Core] Add uniqueId generator May 13, 2016

oliviertassinari reviewed May 13, 2016
View reviewed changes

oliviertassinari mentioned this pull request May 13, 2016

[TextField] Fix server-side rendering #4260

Closed

3 tasks

oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label May 13, 2016

oliviertassinari reviewed May 14, 2016
View reviewed changes

oliviertassinari added PR: Review Accepted and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels May 14, 2016

mbrookes added the Refactoring label May 16, 2016

oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Review Accepted labels May 16, 2016

oliviertassinari added PR: Review Accepted and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels May 17, 2016

mbrookes added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Review Accepted labels May 17, 2016

mbrookes added PR: Needs Review and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels May 18, 2016

mbrookes reviewed May 18, 2016
View reviewed changes

oliviertassinari reviewed May 22, 2016
View reviewed changes

[Core] Add uniqueId generator

d576e5f

oliviertassinari added PR: Review Accepted and removed PR: Needs Review labels May 22, 2016

oliviertassinari mentioned this pull request Jun 9, 2016

[TextField] Fix floatingLabelText intercepting click events #4418

Merged

3 tasks

isaiah mentioned this pull request Jun 10, 2016

idgen incremental ID generator breaks servers-side rendering react-materialize/react-materialize#63

Closed

oliviertassinari force-pushed the master branch from a711bfc to abc85d8 Compare July 26, 2016 20:12

oliviertassinari force-pushed the master branch from 53b828e to ea2538e Compare August 15, 2016 22:46

oliviertassinari mentioned this pull request Oct 4, 2016

[TextField] uniqueId breaks server rendering #3757

Closed

oliviertassinari closed this Oct 4, 2016

zannager added the core Infrastructure work going on behind the scenes label Mar 14, 2023

		```

		Read more details about uniqueId in the [issue](https://github.com/callemall/material-ui/issues/3757) and the [PR](https://github.com/callemall/material-ui/pull/4253).

[Core] Add uniqueId generator #4253

[Core] Add uniqueId generator #4253

Conversation

ajihyf commented May 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajihyf May 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajihyf May 14, 2016 • edited Loading

Choose a reason for hiding this comment

oliviertassinari commented May 14, 2016

ajihyf commented May 14, 2016

mbrookes commented May 16, 2016

ajihyf commented May 17, 2016 • edited Loading

alitaheri commented May 17, 2016

mbrookes commented May 17, 2016

mbrookes commented May 17, 2016

alitaheri commented May 17, 2016

nathanmarks commented May 17, 2016 • edited Loading

alitaheri commented May 17, 2016

ajihyf commented May 18, 2016

ajihyf commented May 18, 2016 • edited Loading

ajihyf commented May 18, 2016

alitaheri commented May 18, 2016

ajihyf commented May 18, 2016 • edited Loading

Choose a reason for hiding this comment

mbrookes commented May 18, 2016 • edited Loading

Choose a reason for hiding this comment

ajihyf May 22, 2016 • edited Loading

Choose a reason for hiding this comment

oliviertassinari May 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari May 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajihyf May 22, 2016 • edited Loading

Choose a reason for hiding this comment

oliviertassinari commented Jun 3, 2016 • edited Loading

ajihyf commented Jun 4, 2016

oliviertassinari commented Jun 4, 2016

alitaheri commented Jun 5, 2016

nathanmarks commented Jun 14, 2016

oliviertassinari commented Jun 16, 2016

nathanmarks commented Jun 16, 2016

oliviertassinari commented Oct 4, 2016

ajihyf commented May 13, 2016 •

edited

Loading

ajihyf May 14, 2016 •

edited

Loading

ajihyf May 14, 2016 •

edited

Loading

ajihyf commented May 17, 2016 •

edited

Loading

nathanmarks commented May 17, 2016 •

edited

Loading

ajihyf commented May 18, 2016 •

edited

Loading

ajihyf commented May 18, 2016 •

edited

Loading

mbrookes commented May 18, 2016 •

edited

Loading

ajihyf May 22, 2016 •

edited

Loading

oliviertassinari May 22, 2016 •

edited

Loading

oliviertassinari May 22, 2016 •

edited

Loading

ajihyf May 22, 2016 •

edited

Loading

oliviertassinari commented Jun 3, 2016 •

edited

Loading