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

Issues/12615 #15473

Closed
wants to merge 3 commits into from
Closed

Conversation

VariableVasasMT
Copy link

@VariableVasasMT VariableVasasMT commented Apr 23, 2019

Before submitting a pull request, please make sure the following is done:

  1. Fork the repository and create your branch from master.
  2. Run yarn in the repository root.
  3. If you've fixed a bug or added code that should be tested, add tests!
  4. Ensure the test suite passes (yarn test). Tip: yarn test --watch TestName is helpful in development.
  5. Run yarn test-prod to test in the production environment. It supports the same options as yarn test.
  6. If you need a debugger, run yarn debug-test --watch TestName, open chrome://inspect, and press "Inspect".
  7. Format your code with prettier (yarn prettier).
  8. Make sure your code lints (yarn lint). Tip: yarn linc to only check changed files.
  9. Run the Flow typechecks (yarn flow).
  10. If you haven't already, complete the CLA.

Learn more about contributing: https://reactjs.org/docs/how-to-contribute.html
For Issue: #12615

@sizebot
Copy link

sizebot commented Apr 23, 2019

ReactDOM: size: 0.0%, gzip: 0.0%

Details of bundled changes.

Comparing: 784ebd8...1f7ca39

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js 0.0% +0.1% 811.37 KB 811.67 KB 185.25 KB 185.37 KB UMD_DEV
react-dom.production.min.js 0.0% 0.0% 103.58 KB 103.63 KB 33.65 KB 33.66 KB UMD_PROD
react-dom.profiling.min.js 0.0% +0.1% 106.73 KB 106.78 KB 34.61 KB 34.63 KB UMD_PROFILING
react-dom.development.js 0.0% +0.1% 805.85 KB 806.15 KB 183.72 KB 183.83 KB NODE_DEV
react-dom.production.min.js 0.0% 0.0% 103.58 KB 103.62 KB 33.1 KB 33.11 KB NODE_PROD
react-dom.profiling.min.js 0.0% 0.0% 106.9 KB 106.94 KB 33.95 KB 33.97 KB NODE_PROFILING
ReactDOM-dev.js 0.0% +0.1% 830.03 KB 830.35 KB 185.06 KB 185.17 KB FB_WWW_DEV
ReactDOM-prod.js 0.0% 0.0% 337.47 KB 337.54 KB 62.69 KB 62.7 KB FB_WWW_PROD
ReactDOM-profiling.js 0.0% 0.0% 342.85 KB 342.92 KB 63.61 KB 63.62 KB FB_WWW_PROFILING
react-dom-unstable-fire.development.js 0.0% +0.1% 811.7 KB 811.99 KB 185.39 KB 185.5 KB UMD_DEV
react-dom-unstable-fire.production.min.js 0.0% 0.0% 103.6 KB 103.64 KB 33.66 KB 33.67 KB UMD_PROD
react-dom-unstable-fire.profiling.min.js 0.0% +0.1% 106.75 KB 106.79 KB 34.62 KB 34.64 KB UMD_PROFILING
react-dom-unstable-fire.development.js 0.0% +0.1% 806.18 KB 806.47 KB 183.86 KB 183.97 KB NODE_DEV
react-dom-unstable-fire.production.min.js 0.0% 0.0% 103.59 KB 103.63 KB 33.11 KB 33.12 KB NODE_PROD
react-dom-unstable-fire.profiling.min.js 0.0% 0.0% 106.91 KB 106.96 KB 33.96 KB 33.98 KB NODE_PROFILING
ReactFire-dev.js 0.0% +0.1% 829.21 KB 829.54 KB 185.06 KB 185.18 KB FB_WWW_DEV
ReactFire-prod.js 0.0% 0.0% 325.44 KB 325.52 KB 60.23 KB 60.24 KB FB_WWW_PROD
ReactFire-profiling.js 0.0% 0.0% 330.79 KB 330.87 KB 61.18 KB 61.19 KB FB_WWW_PROFILING
react-dom-test-utils.production.min.js 0.0% 0.0% 10.31 KB 10.31 KB 3.82 KB 3.82 KB NODE_PROD
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 60.76 KB 60.76 KB 15.84 KB 15.85 KB UMD_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% 0.0% 10.69 KB 10.69 KB 3.67 KB 3.67 KB UMD_PROD
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 60.43 KB 60.43 KB 15.72 KB 15.72 KB NODE_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% 0.0% 10.42 KB 10.42 KB 3.56 KB 3.57 KB NODE_PROD
react-dom-server.browser.production.min.js 0.0% -0.0% 19.18 KB 19.18 KB 7.21 KB 7.21 KB UMD_PROD
react-dom-server.browser.production.min.js 0.0% -0.0% 19.1 KB 19.1 KB 7.2 KB 7.2 KB NODE_PROD
ReactDOMServer-prod.js 0.0% 0.0% 47.14 KB 47.14 KB 10.8 KB 10.8 KB FB_WWW_PROD
react-dom-server.node.production.min.js 0.0% -0.0% 19.96 KB 19.96 KB 7.51 KB 7.51 KB NODE_PROD
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.1% 1.21 KB 1.21 KB 705 B 706 B UMD_PROD
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 3.49 KB 3.49 KB 1.41 KB 1.41 KB NODE_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.2% 1.05 KB 1.05 KB 636 B 637 B NODE_PROD
react-dom-unstable-fizz.node.development.js 0.0% +0.1% 3.74 KB 3.74 KB 1.43 KB 1.43 KB NODE_DEV
react-dom-unstable-fizz.node.production.min.js 0.0% 🔺+0.2% 1.1 KB 1.1 KB 666 B 667 B NODE_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.1% +0.1% 556.46 KB 556.76 KB 121.46 KB 121.57 KB UMD_DEV
react-art.production.min.js 0.0% 0.0% 95.49 KB 95.53 KB 29.31 KB 29.32 KB UMD_PROD
react-art.development.js +0.1% +0.1% 487.49 KB 487.79 KB 104.07 KB 104.18 KB NODE_DEV
react-art.production.min.js 🔺+0.1% 🔺+0.1% 60.46 KB 60.5 KB 18.61 KB 18.63 KB NODE_PROD
ReactART-dev.js +0.1% +0.1% 496.65 KB 496.97 KB 103.13 KB 103.26 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.1% 🔺+0.1% 194.81 KB 194.91 KB 33.14 KB 33.15 KB FB_WWW_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js 0.0% +0.1% 628.2 KB 628.5 KB 133.32 KB 133.45 KB RN_FB_DEV
ReactNativeRenderer-prod.js 🔺+0.1% 🔺+0.2% 243.59 KB 243.83 KB 42.34 KB 42.44 KB RN_FB_PROD
ReactNativeRenderer-profiling.js +0.1% +0.2% 251.77 KB 252.01 KB 43.91 KB 44.01 KB RN_FB_PROFILING
ReactNativeRenderer-dev.js 0.0% +0.1% 628.12 KB 628.41 KB 133.29 KB 133.41 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 🔺+0.1% 🔺+0.2% 243.61 KB 243.84 KB 42.34 KB 42.43 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +0.1% +0.2% 251.79 KB 252.03 KB 43.91 KB 44.01 KB RN_OSS_PROFILING
ReactFabric-dev.js 0.0% +0.1% 616.89 KB 617.18 KB 130.62 KB 130.74 KB RN_FB_DEV
ReactFabric-prod.js 🔺+0.1% 🔺+0.3% 237.36 KB 237.6 KB 41.08 KB 41.19 KB RN_FB_PROD
ReactFabric-profiling.js +0.1% +0.2% 245.02 KB 245.25 KB 42.69 KB 42.79 KB RN_FB_PROFILING
ReactFabric-dev.js 0.0% +0.1% 616.79 KB 617.09 KB 130.58 KB 130.7 KB RN_OSS_DEV
ReactFabric-prod.js 🔺+0.1% 🔺+0.3% 237.37 KB 237.61 KB 41.08 KB 41.18 KB RN_OSS_PROD
ReactFabric-profiling.js +0.1% +0.2% 245.03 KB 245.26 KB 42.69 KB 42.79 KB RN_OSS_PROFILING

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.1% +0.1% 500.9 KB 501.19 KB 106.78 KB 106.89 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.1% 🔺+0.1% 61.68 KB 61.72 KB 18.93 KB 18.94 KB UMD_PROD
react-test-renderer.development.js +0.1% +0.1% 496.44 KB 496.73 KB 105.68 KB 105.79 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.1% 🔺+0.1% 61.36 KB 61.41 KB 18.8 KB 18.81 KB NODE_PROD
ReactTestRenderer-dev.js +0.1% +0.1% 507.22 KB 507.54 KB 105.37 KB 105.49 KB FB_WWW_DEV
react-test-renderer-shallow.development.js 0.0% 0.0% 36.11 KB 36.11 KB 9.36 KB 9.36 KB NODE_DEV
react-test-renderer-shallow.production.min.js 0.0% 0.0% 11.81 KB 11.81 KB 3.67 KB 3.67 KB NODE_PROD

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.1% +0.1% 485.02 KB 485.32 KB 102.5 KB 102.62 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.1% 🔺+0.1% 61.39 KB 61.43 KB 18.35 KB 18.36 KB NODE_PROD
react-reconciler-persistent.development.js +0.1% +0.1% 482.93 KB 483.22 KB 101.62 KB 101.74 KB NODE_DEV
react-reconciler-persistent.production.min.js 🔺+0.1% 🔺+0.1% 61.4 KB 61.44 KB 18.36 KB 18.37 KB NODE_PROD
react-reconciler-reflection.production.min.js 0.0% -0.1% 2.43 KB 2.43 KB 1.09 KB 1.09 KB NODE_PROD

Generated by 🚫 dangerJS

@VariableVasasMT
Copy link
Author

VariableVasasMT commented Apr 23, 2019

I havent added test as of now, because I dont know how to.
partial hydration throws direct error, its hydration through loadable component using ssr, causes this issue, I made changed to ssr fixtures to show that it works, but cant find a way to modify following to make test suite do the same thing

can anyone help?

it('should be able to use lazy components after hydrating with portal', async () => {
    class Child extends React.Component {
      render() {
        if (!document.getElementById('root')) {
          return null;
        }
        const root = document.getElementById('root');
        return ReactDOM.createPortal('World', root);
      }
    }

    const Lazy = React.lazy(
      () =>
        new Promise(resolve => {
          setTimeout(
            () =>
              resolve({
                default: () => <Child />,
              }),
            1000,
          );
        }),
    );
    class HelloWorld extends React.Component {
      state = {isClient: false};
      componentDidMount() {
        this.setState({
          isClient: true,
        });
      }
      render() {
        return (
          <div id="root">
            Hello{' '}
            {this.state.isClient && (
              <React.Suspense fallback="loading">
                <Lazy />
              </React.Suspense>
            )}
          </div>
        );
      }
    }

    const element = document.createElement('div');
    element.innerHTML = ReactDOMServer.renderToString(<HelloWorld />);
    expect(element.textContent).toBe('Hello ');

    ReactDOM.hydrate(<HelloWorld />, element);
    expect(element.textContent).toBe('Hello loading');

    jest.runAllTimers();
    await Promise.resolve();
    Scheduler.flushAll();
    console.log();
    expect(element.textContent).toBe('Hello world');
  });

@VariableVasasMT
Copy link
Author

more so my issue with creating test is that a document has been declared globaly

@cpojer
Copy link
Contributor

cpojer commented Apr 25, 2019

Does it make sense to throw when trying to render a portal on the server or should they just be ignored instead?

@ljharb
Copy link
Contributor

ljharb commented Apr 25, 2019

If they throw, then a component using portal can’t be server rendered - but if they’re ignored, then there’ll be a client-server dom mismatch on initial render. What would be ideal for me is if they’re ignored on the server for rendering, and rendered on the client but ignored for the purposes of the mismatch algorithm, if I’m understanding things correctly.

@VariableVasasMT
Copy link
Author

@cpojer @ljharb on the fair note, this specific issue is not server related, the hydration occurs on client, I dont think if there would be dom mismatch because of it, or may be I dont understand enough.

@ljharb
Copy link
Contributor

ljharb commented Apr 26, 2019

My understanding is that hydration is the initial render on the client on top of the initial server render, and effectively that must not produce a dom diff (altho i think there’s some wiggle room in there to account for subtle html differences). Which means, that if something is rendered in hydration, that i suspect it needs to have also been rendered on the server - or, the diffing algorithm needs to special-case allowing it.

@VariableVasasMT
Copy link
Author

VariableVasasMT commented Apr 29, 2019

diffing algorithm needs to special-case allowing it.

@ljharb I agree with the latter, but is it worth it considering #13097 is open?
As much I understood, HostPortal is not passed to fibre or render tree

case HostPortal:
          pushHostContainer(
            workInProgress,
            workInProgress.stateNode.containerInfo,
          );
          break;

after this we loose the context of Portal, I think but I am not sure,
it should be handled like

case SuspenseComponent: 

which does this

updateDehydratedSuspenseComponent

I am not sure though and I am sorry if this seems naive.

@ljharb
Copy link
Contributor

ljharb commented Apr 29, 2019

I'm not really sure ¯\_(ツ)_/¯ but presumably the first one to land shouldn't presuppose the existence of the other :-)

@VariableVasasMT
Copy link
Author

closing this, as I think this pull request is not important

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.

5 participants