Skip to content

ssr: switch to iterative rendering and don't use process.nextTick in basic renerer (RFC) #6651

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

Closed
wants to merge 4 commits into from

Conversation

wojtask9
Copy link

@wojtask9 wojtask9 commented Sep 20, 2017

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

From comment #5415 (comment)
" in 2.4 we already shipped a pure js build of vue-server-renderer, but we haven't announced it yet. It only supports the base renderer, but is decoupled from Node.js APIs"

That statement is not correct. basic.js still use process.nextTick() when recursion stack is to high.
With this PR switching to iterative version we can avoid process.nextTick() path.
Ideally createBasicRenderer should use different write function but currently this PR is "Request For Comments"

Also iterative version passes all SSR tests (not only basic if we switch to full server-renderer) and helps a lot for not Node.js env (in my case Nashorn)

@wojtask9 wojtask9 changed the title ssr: switch to iterative rendering and don't use process.nextTick (RFC) ssr: switch to iterative rendering and don't use process.nextTick in basic renerer (RFC) Sep 20, 2017
@yyx990803
Copy link
Member

Thanks! This is indeed something we overlooked. The basic renderer was still a proof of concept which is why we didn't include it in the 2.4 release notes. Glad that this is noticed before we make it public :)

However, if we were to make it truly env-agnostic, the basic renderer should not be even able to access process, which is a Node-specific global (maybe it also exists in Nashorn, but not in every environment). The process.env.BASIC_RENDERER needs to be converted into constants during the compilation phase. I'll take a look at this as part of 2.5 features, but feel free to try addressing that.

@wojtask9
Copy link
Author

There isn't process in nashorn. I'm all for env-agnostic basic renderer.

I'll try switch from process.env.... to constants

@yyx990803
Copy link
Member

FYI env vars to constant replacement can be configured here

@wojtask9
Copy link
Author

wojtask9 commented Sep 20, 2017

@yyx990803
Something like this?
Another question. How to fix flow

src/server/render-context.js:64
 64:     if (__BASIC_RENDERER__ === true) {
             ^^^^^^^^^^^^^^^^^^ identifier `__BASIC_RENDERER__`. Could not resolve name

looking for __WEEX__ I can't find anything about in config files

@yyx990803
Copy link
Member

Thanks for looking into this! Instead of making the whole rendering synchronous, we can just use a platform specific deferring function instead. In c5d0fa0 I've made it preferring process.nextTick -> Promise.then -> setTimeout.

In php-v8js Promise is already supported; in Nashorn, you can polyfill setTimeout using Java timers.

@yyx990803 yyx990803 closed this Oct 3, 2017
@wojtask9
Copy link
Author

wojtask9 commented Oct 3, 2017

Just for curiosity. Is there anything wrong with synchronous approach?
In my free time I'm working on Vue SSR in pure java (core still use Nashorn) and if there are some issues with synchronous approach it is better to me know this early :)

@yyx990803
Copy link
Member

Vue's SSR is designed to be async first, because there are many operations that may be async during render:

  • async components
  • external cache calls
  • unwinding deep call stacks

Making things sync is just fundamentally against the design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants