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

[WIP] Streaming server renderer on top of existing client checksum validator #6836

Closed

Conversation

aickin
Copy link
Contributor

@aickin aickin commented May 22, 2016

This is a work-in-progress PR to further the discussion of #6618. It re-implements server rendering in a way that supports streaming and is hopefully maintainable with unit tests.

I'm not asking for this PR to be merged right now. I'm really only opening this PR to allow folks to easily see the diff and evaluate this implementation strategy, as a robust discussion is going on in #6618 about the right way (if any) to achieve streaming render. So I'm going to propose that for now we keep the discussion about this strategy over in #6618. The one exception is if you want to comment on the code, of course feel free to do so here.

If you have any worries/objections about this plan for the discussion, please let me know!

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

  1. Fork the repo and create your branch from master. Done.
  2. If you've added code that should be tested, add tests! Done.
  3. If you've changed APIs, update the documentation. Not done.
  4. Ensure the test suite passes (grunt test). Currently passes, but two tests have been xited out.
  5. Make sure your code lints (grunt lint) - we've done our best to make sure these rules match our internal linting guidelines. Done.
  6. If you haven't already, complete the CLA. Done.

aickin added 30 commits May 15, 2016 22:49
… the unit tests all pass right now. Still more to add.
…basic components section and upgraded several of them to use itRenders
…ator. A bunch of unit tests still fail (19 in server render test and more overall).
…r warning messages disable client warning messages.
…ive tests are broken; will be fixed on next merge from test branch.
@@ -25,6 +25,8 @@ module.exports = function() {
entries: entries,
debug: config.debug, // sourcemaps
standalone: config.standalone, // global
builtins: {},
detectGlobals: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this incantation is necessary to stop browserify from bundling in a fake version of node's stream library.

@aickin
Copy link
Contributor Author

aickin commented May 22, 2016

From travis, it looks like there's 2 test failures for CSS warnings when running the tests in SSR mode. The CSS error messages don't include the name of the component that has the issue, but otherwise the error messages are correct.

I'll probably not fix this right now, as it looks like I'd have to implement component ownership to get the correct component name, and I'd rather wait until it's clear that this is the way to go forward.

… that don't have special characters. The other is an inlining of creating the data-reactid
@ghost
Copy link

ghost commented May 31, 2016

@aickin updated the pull request.

@aickin
Copy link
Contributor Author

aickin commented Jun 25, 2017

After the merging of #10024, this PR is obsolete. Closing this PR with thanks to @spicyj, @tomocchino, and @sebmarkbage for all they did to pave the way for streaming rendering to make it into core.

@aickin aickin closed this Jun 25, 2017
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.

2 participants