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

RFC: Exposing Prismic Events #68

Closed
lihbr opened this issue Aug 20, 2020 · 7 comments
Closed

RFC: Exposing Prismic Events #68

lihbr opened this issue Aug 20, 2020 · 7 comments

Comments

@lihbr
Copy link
Member

lihbr commented Aug 20, 2020

Summary

Making some Prismic events accessible through an interface.

Examples

Examples with window.prismic.addEventListener and window.prismic.removeEventListener as an interface.

Refreshing Nuxt.js without a hard reload (or with any framework capable of behaving like an SPA):

window.prismic.addEventListener("update_preview", (e) => {
  e.preventDefault();
  this.$nuxt.refresh();
});

Closing preview session cookies on Next.js (would fix #64):

window.prismic.addEventListener("close_preview_session", () => res.clearPreviewData());

Motivation

Exposing some Prismic events would allow users to extend or alter previews or other features behaviors of the toolbar, therefore allowing improved experience.

Implementation

The "hacky" way would be to just expose this.events, addEventListener and removeEventListener methods in it although as pointed out it's definitely better to work on a proper, public, event layer.

Preventing Default Behavior

For some events like update_preview users need to take ownership of default behavior (location hard reload), therefore a preventDefault API should be included.

Events

TBD

Old & not relevant anymore

Unknowns

  • I'm not really sure about the event life-cycle happening between the iframe and the client
  • For update_preview event, if preview ref needs to be updated in the process it could make the SPA refresh harder to achieve 🤔
  • Tracking could be messed up
  • update_preview event is currently not an event, therefore it needs to be reworked into one (or wrapped and fired on if (reload))
@arnaudlewis
Copy link
Member

We can talk about it but I would not expose events like this, it would be a complete hack to me, we need a proper channel for this kind of use.
There is a whole protocol between the client and the iframe for a good reason, I want people to respect the lifecycle without being able to mess up everything meaning that they need a dedicated interface to interact with prismic if it becomes something that we want.

@lihbr
Copy link
Member Author

lihbr commented Aug 20, 2020

Yes that's what I felt too, let's think about a dedicated "public" event layer then 🎉

Updated RFC to reflect that~

@lihbr lihbr changed the title RFC: Expose iframe events RFC: Exposing Prismic Events Aug 20, 2020
@lihbr
Copy link
Member Author

lihbr commented Aug 21, 2020

Following what we talked about this morning:

Having events could be misleading to users and could lead to more harm than anything because of toolbar complexity. The middleware approach sounds safer and more obvious to use. These middlewares would be executed if found inside a window.prismic.middleware (TBD) object.

We'll start by working on two events:

update_preview

Location: https://github.com/prismicio/prismic-toolbar/blob/master/src/toolbar/preview/index.js#L42
Expected usage (example, refreshing Nuxt the SPA way), this happens here:

window.prismic.middleware.previewUpdate = (event, next) => {
  this.$nuxt.refresh();
  // `next()` is not called because we want to override default behavior here 
}

close_preview_session

Location: https://github.com/prismicio/prismic-toolbar/blob/master/src/toolbar/preview/index.js#L63
Expected usage (example, closing Next.js preview), this happens here to me to also allow and SPA refresh if wanted:

window.prismic.middleware.closePreviewSession = (event, next) => {
  res.clearPreviewData();
  next();
}

Unknowns

  • Is next function needed? We can get rid of it but that sounds misleading to me as in the first case we want to prevent what comes after and on the second we don't 🤔

@veloce
Copy link
Contributor

veloce commented Aug 25, 2020

Instead of window.prismic.middleware which can be misleading (it's not exactly middleware here), why not using instead a custom pubsub system (very light), whose interface would be something like (perhaps named differently):

window.prismic.events.on('closePreviewSession', (data) => {
   // do what you want
})

That way you expose only two functions on, and off and you don't need to change the prismic object when adding new events.

Implementation could be as simple as:

// utils/pubsub.js
const events = new Map()

// also exported in window.prismic
export function on(name, cb) {
  const cbs = events.get(name)
  if (cbs) cbs.add(cb)
  else events.set(name, new Set([cb]))
}

// also exported in window.prismic
export function off(name, cb) {
  const subs = events.get(name)
  if (subs) {
    subs.delete(cb)
  }
}

export function dispatch(name, data) {
  const subs = events.get(name)
  if (!subs) return
  for (const s of subs) {
    s(data)
  }
}

@arnaudlewis
Copy link
Member

I agree on the middleware name it was temporary to make a point but I'm not a fan of pubsub here, first managing event with on/off and a string event is more error prone to me, it's not as declarative and easy to sort things out on your own.
That would work for a private stuff but here i think it just makes this system more obscur to people.
You could subscribe to any event, even non existing one meaning errors + a doc necessary for each event.
With functions, it's almost self explanatory

@veloce
Copy link
Contributor

veloce commented Aug 25, 2020

Yes it's true, this is the drawback of pubsub. In that case exposing functions is better, I agree.

@lihbr
Copy link
Member Author

lihbr commented May 23, 2022

Shipped in #93

@lihbr lihbr closed this as completed May 23, 2022
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

No branches or pull requests

3 participants