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

Immutable state on SSR (toString) #649

Merged
merged 5 commits into from
Jun 12, 2019

Conversation

tornqvist
Copy link
Member

@tornqvist tornqvist commented Mar 22, 2018

This PR solves state leaking between calls to toString and race conditions that occur during double render pass (bankai et al.).

The issue is that each call to toString would extend app.state, adding on more and more data for each render. If one was to expose the resulting app.state as window.initalState one would get the combined state from all prior calls to toString.
It is especially troublesome during double render pass (calling toString twice, once to allow events to trigger data being fetched and another to render the view with fetched data). If two attempts at a double render pass are issued in quick succession (e.g. server responding to several requests), both calls will mutate app.state and the first to finish will get the intermediate state of both render calls.

This PR ensures that the state passed into toString is only modified during that one pass through and that app.state remains untouched.

This would also allow us to proceed with choojs/bankai#446

Edit: Yet another issue solved by this PR is that every call to toString would initialize all stores and hence, add all listeners to the event bus over and over again for every render. This means that after a couple of renders any event listeners that e.g. fetches data will be fetching the same data several times over on every call to toString.

@tornqvist tornqvist changed the title Init stores with state Immutable state on SSR (toString) Jul 5, 2018
* master:
  6.13.3
  Revert init order change
  Add browser tests (choojs#696)
  6.13.2
  Fix location missing on store init (choojs#695)
  Typo Fix in Readme (choojs#693)
  6.13.1
  Fix wrong this usage in nanohref integration (choojs#689)
  Some spelling & typo fixes in readme (choojs#688)
  Fix inspect script (choojs#654)
@tornqvist tornqvist mentioned this pull request Jun 8, 2019
6 tasks
* master:
  Update nanorouter (choojs#701)
  Use Object.assign instead of xtend (choojs#616)
@bcomnes
Copy link
Collaborator

bcomnes commented Jun 12, 2019

LGTM. Any weird edge cases or anomalies this could cause for people upgrading that we need to mention in a breaking change log?

@tornqvist
Copy link
Member Author

The only real world effect it would have is for people and frameworks doing SSR and then plucking out e.g. title from the state. (I have prepared a PR to bankai for this: choojs/bankai#527)

var html = app.toString('/', state)
- var title = app.state.title
+ var title = state.title

The only other thing I can think of is that stores will now only have their event listeners triggered once per event (as opposed to accumulating listeners for each toString) and app.state will not be populated during toString. I'll make a point of mentioning this in the change log.

* master:
  Match route before init stores (choojs#698)
  more timings (choojs#578)
  Disable hash routing by default (choojs#699)
@tornqvist tornqvist merged commit 842b7aa into choojs:master Jun 12, 2019
@tornqvist tornqvist deleted the init-stores-with-state branch June 12, 2019 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants