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

AXAContainer + react-router + children problem #859

Closed
MarekLacoAXA opened this issue Mar 8, 2019 · 13 comments
Closed

AXAContainer + react-router + children problem #859

MarekLacoAXA opened this issue Mar 8, 2019 · 13 comments

Comments

@MarekLacoAXA
Copy link
Contributor

Problems using AXAContainer / AXARow / AXACol experienced:

AXAContainer

Rerender, children passed to <AXAContainer> change, but the output keeps being the same.

axa-container

Rerender, children passed to <axa-container> change, react crashes:

image

@LucaMele
Copy link
Contributor

LucaMele commented Mar 8, 2019

Hi makre, thank you for submitting this. I belive this is the issue with the Children of Children dynmaic rendering. We can only fix this in v2 unfortunatly.

@MarekLacoAXA
Copy link
Contributor Author

Hi @LucaMele ! Just wonder whether this kind of error is solvable at all with the current status of React and Web Components. What do you think?

@LucaMele
Copy link
Contributor

LucaMele commented Mar 11, 2019

We are not the first one doing this. Frameworks like Ionic also solved it so im pretty sure is solvable. So far in our tests it did work with the library skatejs/val. Its a library that syncs vdom and dom. For controled input instead, we do the same as react does, setting and resetting after.

All tests we did are very promising so far. U can checkout develop-v2 branch.

But if its not solvable, we will write 2 components: 1 in react and 1 as custom element. Therefore for you, when u import the component, it will definitely work.

@markus-walther
Copy link
Contributor

@MarekLacoAXA I will be the one to do an experiment to show this works in v2. Could you help me understand your use case by attaching a minimal React code snippet that shows the problem (in v1, of course)?

@AndyOGo
Copy link

AndyOGo commented Mar 12, 2019

@LucaMele
@MarekLacoAXA
@markus-walther
Indeed very likely that this is related to the dynamic children issue described here #778
It includes a code snippet with and without the withReact integration helper (both are failing).
So my findings are that:

This PR contains the reproduction test scenario:
https://github.com/axa-ch/patterns-library-examples/pull/16

@MarekLacoAXA
Copy link
Contributor Author

Hi @markus-walther !

I was unable to reproduce this issue without using react-router. Therefore I think, this issue is related to react-router and the children props. See the withRouter() function - it behaves the children a special way.

Here is my minimalistic error example. It generates two timestamps. Navigating to page "foo" and "bar" should refresh both timestamps. But the timestamp enclosed in AXACol doesn't change.

import { AXACol } from '@axa-ch/patterns-library/lib/js/react-exports';
import * as React from 'react';
import * as ReactDOM from 'react-dom';
import { withRouter } from 'react-router';
import { BrowserRouter } from 'react-router-dom';

const CurrentTime = () => (
  <strong>
    {new Date().toLocaleString()}
  </strong>
);

class Content extends React.Component {
  public render() {
    return (
      <>
        <div onClick={() => this.props.history.push('/foo')}>foo</div>
        <div onClick={() => this.props.history.push('/bar')}>bar</div>
        {this.props.children}
      </>
    );
  }
}

const ContentWithRouter = withRouter(Content);

export class App extends React.Component {
  public render() {
    return (
      <BrowserRouter>
        <ContentWithRouter>
          <AXACol>
            <CurrentTime/>
          </AXACol>
          <CurrentTime/>
        </ContentWithRouter>
      </BrowserRouter>
    );
  }
}

ReactDOM.render(
  <App/>,
  document.getElementById('root'),
);

@MarekLacoAXA MarekLacoAXA changed the title axa-container / AXAContainer / React problem? AXAContainer + react-router + children problem Mar 14, 2019
@markus-walther
Copy link
Contributor

markus-walther commented Mar 18, 2019

Thanks @MarekLacoAXA ! As discussed privately, the belief is that "[...]any component using the children prop would be affected.", i.e. also the original <AXAContainer>.

@AndyOGo
Copy link

AndyOGo commented Mar 20, 2019

@MarekLacoAXA
@LucaMele
@markus-walther
I created a test case rebuilding the Grid components for both native <custom-elements> and lit-element elements and using them inside React with the suggested @skatejs/val wrapper for V2.

Tests:

Todo:

  • Test it with above react-router case - @MarekLacoAXA may I ask you for help to reproduce your issue?

Publicly available at:
https://codesandbox.io/s/l4jkw43q7

Please share your thoughts.

From my point of view, this is good enough, only the events: { ... } syntax is uncommon for React.
Maybe the @skatejs/val wrapper can be extended to automatically collect react events 🤔

Personally I'm still keen to find out why React's event system can't handle these custom element nodes 😕

@MarekLacoAXA
Copy link
Contributor Author

MarekLacoAXA commented Mar 28, 2019

Sorry, my Example from March 14 was too complicated.
Here is a better, simpler, example thanks to @markus-walther

Problem:
When clicking on links foo/bar, the content does not change.
When replacing <AXACol> with a plain <div>, is does change, as expected.

import { AXACol } from '@axa-ch/patterns-library/lib/js/react-exports';
import * as React from 'react';
import * as ReactDOM from 'react-dom';
import { Route, Switch } from 'react-router';
import { BrowserRouter, Link } from 'react-router-dom';

class App extends React.Component {
  public render() {
    return (
      <BrowserRouter>
        <>
          <Link to="/foo">
            foo
          </Link>
          |
          <Link to="/bar">
            bar
          </Link>
          <AXACol>
            <Switch>
              <Route path="/foo" component={() => <div>FOO</div>}/>
              <Route path="/bar" component={() => <div>BAR</div>}/>
            </Switch>
          </AXACol>
        </>
      </BrowserRouter>
    );
  }
}


ReactDOM.render(
  <App/>,
  document.getElementById('root'),
);

@AndyOGo
Copy link

AndyOGo commented Mar 28, 2019

@MarkLacoAXA
Thanks for the example, we should add this to the skatejs/val test i shared above

@LucaMele
Copy link
Contributor

@MarekLacoAXA, @markus-walther is working on it and we have our own test environment. Please contact directly hin

@AndyOGo
Copy link

AndyOGo commented Mar 28, 2019

@MarekLacoAXA
@markus-walther
@LucaMele
I added above scenario.
It works for me (I did not test with plib V1 components)
https://codesandbox.io/s/64y98m1w5k

PS: all of you can edit and extend these tests, feel free to do so

PPS: I just added the plib V1 components out of curiosity, and they are broken even with skate/val 😞
I would be very keen to find out why, I assume it could be our monkey patching HOC, nanohtml or nanomorph for incremental rendering, or anything else 🤔

@AndyOGo
Copy link

AndyOGo commented Apr 15, 2019

@MarekLacoAXA
@markus-walther
@LucaMele
I just created a public test case with the newest V1 release 2.0.1-beta.264 with #913 merged and this works now as expected :)
https://codesandbox.io/s/lrxj3vk0nz

If all of you agree I close this ticket?

@AndyOGo AndyOGo closed this as completed Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants