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

ui: Small EventSource additions #5978

Merged
merged 1 commit into from
Jun 20, 2019

Conversation

johncowen
Copy link
Contributor

  1. EventSources now pass themselves thorugh to the run function as a
    second argument. This enables the use of arrow functions along with the
    EventSources API
    (configuration, source) => source.close().
    Order of arguments could potentially be switched at a later date.

  2. BlockingEventSources now let you pass an 'event' through at
    instantiation time. If you do this, the event will immediately be dispatched
    once the EventSource is opened. The usecase for this is for 'unfreezing' cached
    BlockingEvents. This makes it easier to provide a cache for
    BlockingEventSources by caching its data rather than the entire
    BlockingEventSource itself.

new BlockingEventSource(
  (config, source) => { /* something */ },
  {
    cursor: 1024, // this would also come from a cache
    currentEvent: getFromSomeSortOfCache(eventSourceId) //this is the new bit
  }
);

// more realistically

new BlockingEventSource(
  (config, source) => asyncData.findSomething(slug, config),
  getFromSomeSortOfCache(eventSourceId)
);

1. EventSources now pass themselves thorugh to the run function as a
second argument. This enables the use of arrow functions along with the
EventSources API
`(configuration, source) => source.close()`. Order of arguments could
potentially be switched at a later date.
2. BlockingEventSources now let you pass an 'event' through at
instantation time. If you do this, the event will immediately be dispatched
once the EventSource is opened. The usecase for this is for 'unfreezing' cached
BlockingEvents. This makes it easier to provide a cache for
BlockingEventSources by caching its data rather than the entire
BlockingEventSource itself.

```
new BlockingEventSource(
  (config, source) => { /* something */ },
  {
    cursor: 1024, // this would also come from a cache
    currentEvent: getFromSomeSortOfCache(eventSourceId) //this is the new bit
  }
);

// more realistically

new BlockingEventSource(
  (config, source) => { return data.findSomething(slug, config) },
  getFromSomeSortOfCache(eventSourceId)
);

```
@johncowen johncowen requested a review from a team June 17, 2019 12:54
@johncowen johncowen added the theme/ui Anything related to the UI label Jun 17, 2019
Copy link

@DingoEatingFuzz DingoEatingFuzz left a comment

Choose a reason for hiding this comment

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

The code looks fine, but I'd like to hear more about unfreezing cached BlockingEvents. I don't follow what causes a blocked event to be frozen, what it means to unfreeze one, and why it's now desired to unfreeze one immediately.

@johncowen
Copy link
Contributor Author

johncowen commented Jun 20, 2019

Potentially used a word that can mean different things here 😬

When I say unfreeze, I mean 'take it out of the cache'. Currently once you leave a page with an EventSource and it is closed, it is placed in a cache to be 'unfrozen' when you next visit that page. We do this so we keep a track of the cursor/index and the previous set of data for immediate displaying.

Currently when we 'cache' EventSources we cache the entire thing, so the EventSource itself - it basically had state, that was a tiny bit more difficult to decouple off from the EventSource itself in order to cache. This moves things to make it a tiny bit easier to just cache the state of the EventSource. It's one of the reasons why I don't like that 'knitting things together' code we spoke about a while ago.

So, this change makes it easier to create a new EventSource with the state of another, therefore you can just cache its state as a JSON blob rather than the EventSource itself. As well as it being slightly easier, I just prefer it, it means I shouldn't need to concern myself with any type of specialised cache just for this (so say event-source/cache.js). Also, as it's just a JSON blob it also means I can potentially use something more than a in-memory cache, say localStorage to cache the state between tabs/browser restarts, meaning 'immediate' initial loading of the UI on revisits. This isn't something we are going to do right now as there are other questions around doing that, but at least this makes it easier to do.

I'd made the state of the cursor externally cacheable already and was heading towards doing all this initially, but didn't realize when I started building it I'd need to make the current data cacheable also and ended up with these getCurrentEvent and setCurrentEvent methods which I don't like. I should be able to use these methods less moving forwards and eventually get rid of them.

and why it's now desired to unfreeze one immediately.

Previously we still did it immediately, just you had to set that up manually from outside the EventSource. It doesn't make sense that you have to set this up manually. So now, if you add a currentEvent to the initial configuration object, it will immediately fire that for you, as if it had come from the backend API. That way when you visit a page, take the EventSource out of the cache, the page immediately renders as if it had received the response immediately. As a result of this change you have to do less to make this happen.

I suppose the TLDR is these are all just tiny tweaks to make it ever so slightly nicer to work with.

P.S. Let me know if I've not covered anything and I can expand here or offline.

@johncowen johncowen merged commit 9b796da into ui-staging Jun 20, 2019
@johncowen johncowen deleted the feature/ui-event-source-additions branch June 20, 2019 09:23
@DingoEatingFuzz
Copy link

Cool cool, thank you for the added color, this makes sense now. Also generally +1 to caching the smallest and simplest required state.

johncowen added a commit that referenced this pull request Jun 21, 2019
1. EventSources now pass themselves thorugh to the run function as a
second argument. This enables the use of arrow functions along with the
EventSources API
`(configuration, source) => source.close()`. Order of arguments could
potentially be switched at a later date.
2. BlockingEventSources now let you pass an 'event' through at
instantation time. If you do this, the event will immediately be dispatched
once the EventSource is opened. The usecase for this is for 'unfreezing' cached
BlockingEvents. This makes it easier to provide a cache for
BlockingEventSources by caching its data rather than the entire
BlockingEventSource itself.

```
new BlockingEventSource(
  (config, source) => { /* something */ },
  {
    cursor: 1024, // this would also come from a cache
    currentEvent: getFromSomeSortOfCache(eventSourceId) //this is the new bit
  }
);

// more realistically

new BlockingEventSource(
  (config, source) => { return data.findSomething(slug, config) },
  getFromSomeSortOfCache(eventSourceId)
);

```
johncowen added a commit that referenced this pull request Aug 22, 2019
1. EventSources now pass themselves thorugh to the run function as a
second argument. This enables the use of arrow functions along with the
EventSources API
`(configuration, source) => source.close()`. Order of arguments could
potentially be switched at a later date.
2. BlockingEventSources now let you pass an 'event' through at
instantation time. If you do this, the event will immediately be dispatched
once the EventSource is opened. The usecase for this is for 'unfreezing' cached
BlockingEvents. This makes it easier to provide a cache for
BlockingEventSources by caching its data rather than the entire
BlockingEventSource itself.

```
new BlockingEventSource(
  (config, source) => { /* something */ },
  {
    cursor: 1024, // this would also come from a cache
    currentEvent: getFromSomeSortOfCache(eventSourceId) //this is the new bit
  }
);

// more realistically

new BlockingEventSource(
  (config, source) => { return data.findSomething(slug, config) },
  getFromSomeSortOfCache(eventSourceId)
);

```
johncowen added a commit that referenced this pull request Aug 29, 2019
1. EventSources now pass themselves thorugh to the run function as a
second argument. This enables the use of arrow functions along with the
EventSources API
`(configuration, source) => source.close()`. Order of arguments could
potentially be switched at a later date.
2. BlockingEventSources now let you pass an 'event' through at
instantation time. If you do this, the event will immediately be dispatched
once the EventSource is opened. The usecase for this is for 'unfreezing' cached
BlockingEvents. This makes it easier to provide a cache for
BlockingEventSources by caching its data rather than the entire
BlockingEventSource itself.

```
new BlockingEventSource(
  (config, source) => { /* something */ },
  {
    cursor: 1024, // this would also come from a cache
    currentEvent: getFromSomeSortOfCache(eventSourceId) //this is the new bit
  }
);

// more realistically

new BlockingEventSource(
  (config, source) => { return data.findSomething(slug, config) },
  getFromSomeSortOfCache(eventSourceId)
);

```
johncowen added a commit that referenced this pull request Sep 4, 2019
1. EventSources now pass themselves thorugh to the run function as a
second argument. This enables the use of arrow functions along with the
EventSources API
`(configuration, source) => source.close()`. Order of arguments could
potentially be switched at a later date.
2. BlockingEventSources now let you pass an 'event' through at
instantation time. If you do this, the event will immediately be dispatched
once the EventSource is opened. The usecase for this is for 'unfreezing' cached
BlockingEvents. This makes it easier to provide a cache for
BlockingEventSources by caching its data rather than the entire
BlockingEventSource itself.

```
new BlockingEventSource(
  (config, source) => { /* something */ },
  {
    cursor: 1024, // this would also come from a cache
    currentEvent: getFromSomeSortOfCache(eventSourceId) //this is the new bit
  }
);

// more realistically

new BlockingEventSource(
  (config, source) => { return data.findSomething(slug, config) },
  getFromSomeSortOfCache(eventSourceId)
);

```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants