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

Ignore noscript content on the client #13537

Merged
merged 3 commits into from Sep 3, 2018
Merged

Conversation

@Ephem
Copy link
Contributor

@Ephem Ephem commented Sep 2, 2018

Fixes #11423

First PR, CLA is signed.

Before this PR <noscript><img src="..." /></noscript> would:

  • Warn about a server/client mismatch
  • Download the img
  • (Run any lifecycles for components inside the noscript)

I tried fixing this by only touching ReactDOM and found that by treating <noscript> as an element that only supports text it achieved the desired effects. This also keeps the markup in the DOM on the client which is desirable as per the issue.

Questions:

  • Is this a hacky way to do it? In a sense shouldSetTextContent is true for noscript-elements on the client, but could it have any unintended side-effects now or in the future?
  • Solving it this way means that the content inside a noscript could still be updated on the client as long as children is a string, is the desirable or confusing?
  • Is the test a sane and robust way to test for this?
@pull-bot
Copy link

@pull-bot pull-bot commented Sep 2, 2018

Details of bundled changes.

Comparing: 28b9289...eb7d792

react-scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-scheduler.development.js +24.4% +23.6% 15.4 KB 19.17 KB 4.65 KB 5.74 KB UMD_DEV
react-scheduler.production.min.js 🔺+15.8% 🔺+21.0% 2.73 KB 3.16 KB 1.26 KB 1.53 KB UMD_PROD

Generated by 🚫 dangerJS

Loading

@gaearon
Copy link
Member

@gaearon gaearon commented Sep 2, 2018

Ideally you might want to try writing an extra test in ReactDOMServerIntegration*-test suite which runs a variety of scenarios for each test.

Loading

@Ephem
Copy link
Contributor Author

@Ephem Ephem commented Sep 2, 2018

Thank you for the quick feedback, I'll have a look at that test suite and see where I can add some more tests.

First ci-fail was a bad test that for some reason worked locally, fixed now. Not sure about the current results.json fail though.

Loading

@Ephem
Copy link
Contributor Author

@Ephem Ephem commented Sep 2, 2018

Added another test. Seeing how tests in ServerIntegration seem to only test output I followed that pattern.

The existing test also tests that render on components inside of <noscript> only gets called on the server, this is kind of a heuristic for if any sideeffects of the component are triggered. One could also test the same for the constructor or any/all of the lifecycles but I'm not sure if that would be helpful?

Loading

odykyi
odykyi approved these changes Sep 3, 2018
gaearon
gaearon approved these changes Sep 3, 2018
@gaearon
Copy link
Member

@gaearon gaearon commented Sep 3, 2018

I think this makes sense to me. Thanks.

Loading

@gaearon gaearon merged commit 0b74e95 into facebook:master Sep 3, 2018
1 check passed
Loading
@gaearon gaearon mentioned this pull request Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants