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

On forceUpdate and connectedElements #2

Closed
mcjazzyfunky opened this issue Sep 16, 2021 · 6 comments
Closed

On forceUpdate and connectedElements #2

mcjazzyfunky opened this issue Sep 16, 2021 · 6 comments

Comments

@mcjazzyfunky
Copy link

mcjazzyfunky commented Sep 16, 2021

[Edit - November 2021]

This proposal was not very helpful, it can/should be ignored. For a much simpler proposal please see
#2 (comment)


That whole Shoelace localization stuff looks pretty awesome.
But what really, really bugs me is that forceUpdate and connectedElements are part of the public API and especially that they are SL specific. I think this is not really necessary and other custom element libraries could use the same "lang attribute observer" strategy as SL without depending on it.
BTW, at the moment, components cannot subscribe for lang change notifications, this should also be fixed with the proposal below.

I've not tested the code but my little brain tells me, something like this could work.
The idea is that a component asks on connect whether there's already someone who's responsible for all this lang attribute observation and lang change notification stuff ... if yes, fine, then the component receives the forceUpdate function and also can subscribe for lang changes ... if no, the component itself will take care that someone will handle that from now on.

I guess, the code might not be easily understandable, yet a bit simplified (and also be full of bugs 😉), but like said, something like that may work:

let forceUpdate: (() => void) | null = null
let cleanup1: (() => void) | null = () => {} // set because of TS code analyzer issues
let cleanup2: (() => void) | null = null

// === on connect =================================================

document.dispatchEvent(new CustomEvent('lang-change-subscription', {
  detail: (notify: () => void, unsubscribe: () => void) => {
    forceUpdate = notify
    cleanup1 = unsubscribe
  
    return () => { refreshSomehow(this) }
  }
}))

if (!forceUpdate) {
  const subscribers = new Set<() => void>([() => refreshSomehow(this)])
  forceUpdate = () => subscribers.forEach(it => it())
  
  new MutationObserver(forceUpdate).observe(document, {
    attributes: true,
    attributeFilter: ['lang'],
    // childList: true, // I don't think we need this
    subtree: true
  })
  
  document.addEventListener('lang-change-subscription', (ev: ...) => {
    const subscriber: () => void =
      ev.detail(forceUpdate, () => subscribers.delete(subscriber))
      
    subscribers.add(subscriber)
  })
}

const observer = new MutationObserver(forceUpdate)

observer.observe(this, {
  attributes: true,
  attributeFilter: ['lang']
})

cleanup2 = () => observer.disconnect()

// === on disconnect ==============================================

cleanup1()
cleanup2()

@claviska What do you think?

[Edit] Maybe it would be nice if the notification callback would also pass the current language of the element in question, anyway ...

@claviska
Copy link
Member

But what really, really bugs me is that forceUpdate and connectedElements are part of the public API and especially that they are SL specific.

How do you mean? It's a separate library and only shares the @shoelace namespace on npm.

The idea is that a component asks on connect whether there's already someone who's responsible for all this lang attribute observation and lang change notification stuff

As the module is independent, a component could subscribe to it using the connectedElements map exposed by the library. That's all the decorators really do — add the element to the map on connect and remove it on disconnect.

It's probably not documented as well as it could be. There's a lot going on and it's a hard concept to describe quickly. But there's probably still room to improve. I'm not sure an event is necessary though, as if the element is connected to the DOM and entered into the connectedElements map, it will receive update when any lang changes.

@mcjazzyfunky
Copy link
Author

mcjazzyfunky commented Sep 17, 2021

How do you mean? It's a separate library and only shares the @Shoelace namespace on npm.

Sure. I should have said SLL 😉 The point is that if 3rd party components want the same "lang detection and auto-update" functionality as provided by Localize then Localize has to be a dependency. It is currently not possible to make 3rd party components first-class citizens for Localize based components (regarding the auto-update feature) without having Localize as dependency (it's not really important that Localize is quite small). This cuts the freedom of choice whether someone wants to use Localize or not. If you instead use something similar to the above mentioned solution then everybody can either use Localize directly or provide h*er own completely independent implementation - both will work exactly the same (if implemented properly). I think this is a big advantage. The really important part is the ideas behind Localize, not necessary any implementation of these ideas.
Nevermind, it's not THAT important for the moment, maybe we can reconsider this this proposal when Localize is feature-complete or whatever.

FYI: Here's an additional implementation (which seems to work for my demos). [Edit]: Fixed a lot of issues of the first versions
// === exports =======================================================

export { detectLocale, observeLocale }

// === constants / module data =======================================

const LOCALE_OBSERVATION = 'locale-observation'
const localeByElem = new Map<HTMLElement, string | null>()

// === types =========================================================

type LocaleObservationApi = {
  subscribe(subscriber: () => void): () => void
  notify(): void
}

type LocaleObservationEvent = {
  type: typeof LOCALE_OBSERVATION
  detail(api: LocaleObservationApi): void
}

declare global {
  interface DocumentEventMap {
    [LOCALE_OBSERVATION]: LocaleObservationEvent
  }
}

// === default api ===================================================

const defaultApi = ((): LocaleObservationApi => {
  let initialized = false
  const subscribers = new Set<() => void>()
  const notify = () => subscribers.forEach((it) => it())

  return {
    subscribe(subscriber) {
      if (!initialized) {
        new MutationObserver(notify).observe(document, {
          attributes: true,
          attributeFilter: ['lang'],
          subtree: true
        })
      }

      const sub = () => subscriber()
      subscribers.add(sub)
      initialized = true
      return () => subscribers.delete(sub)
    },
    notify
  }
})()

// === locale observation functions ==================================

const { subscribe, notify } = ((): LocaleObservationApi => {
  let api: LocaleObservationApi | null = null

  document.dispatchEvent(
    new CustomEvent(LOCALE_OBSERVATION, {
      detail: (otherApi: LocaleObservationApi) => (api = otherApi)
    })
  )

  if (!api) {
    api = defaultApi
    document.addEventListener(LOCALE_OBSERVATION, (ev) => ev.detail(defaultApi))
  }

  return api
})()

// === getLocale =====================================================

function getLocale(elem: HTMLElement): string | null {
  let el: Element = elem

  while (el && !(el instanceof Window) && !(el instanceof Document)) {
    const next = el.closest<HTMLElement>('[lang]')

    if (next) {
      return next.lang || null
    }

    el = (el.getRootNode() as ShadowRoot).host
  }

  return null
}

// === detectLocale ==================================================

function detectLocale(elem: HTMLElement) {
  const locale = localeByElem.get(elem)
  return locale !== undefined ? locale : getLocale(elem)
}

// === observeLocale =================================================

function observeLocale(elem: HTMLElement, callback: () => void) {
  let locale: string | null = null
  let cleanup1: (() => void) | null = null
  let cleanup2: (() => void) | null = null

  return {
    connect() {
      locale = getLocale(elem)

      cleanup1 = subscribe(() => {
        locale = getLocale(elem)
        localeByElem.set(elem, locale)
        callback()
      })

      if (elem.getRootNode() instanceof ShadowRoot) {
        const observer = new MutationObserver(notify)

        observer.observe(elem, {
          attributes: true,
          attributeFilter: ['lang']
        })

        cleanup2 = () => observer.disconnect()
      }
    },

    disconnect() {
      localeByElem.delete(elem)
      cleanup1 && cleanup1()
      cleanup2 && cleanup2()
      cleanup1 = cleanup2 = null
    },

    getLocale: () => locale
  }
}

@claviska
Copy link
Member

It is currently not possible to make 3rd party components first-class citizens for Localize based components (regarding the
auto-update feature) without having Localize as dependency

This is counterintuitive. You want something to work as a first-class citizen, but you don't want to subscribe to its usage.

This package really does two things:

  1. It provides a mechanism for detecting the language of an arbitrary element based on its position in the DOM and its ancestors' lang attributes
  2. It informs components when lang changes so they can automatically re-render

Neither of these are provided for us by the platform, hence this library. What you're proposing introduces a non-standard API and relies on a custom event that other libraries would have to listen for. I don't see any advantage to that. It's just a different approach.

Before trying to solve whatever problem you're trying to solve — which, admittedly, isn't very clear to me — you might think about what you really want to have for a higher level API.

I chose lang because it's semantically invalid to render something contained by it in anything but the specified language. Thus, if you want lang to dictate the language elements should render, you've come to the right place. On the other hand, if you want a custom event to be the de facto thing an element listens for, this library probably isn't for you.

Whether or not a project wants to use this as a dependency isn't a priority for me. 🤷🏻‍♂️

@mcjazzyfunky
Copy link
Author

Okay, thanks ... then I'll close this issue.

@mcjazzyfunky
Copy link
Author

mcjazzyfunky commented Sep 18, 2021

Just for the record (no need to answer) as this was obviously not clear and I do not want to appear impolite or whatever not clarifing it:
The above proposal tried to provide an API for the (of course) same requirements as Localize but just in a library agnostic manner and trying to avoid exposing low-level stuff like forceUpdate and connectedElements (whether the latter may be a good idea or not is another question), plus auto-registering all necessary MutationObservers (also those for the observed elements in the shadow DOM - hoping to increase DX a bit):

const obs = observeLocale(elem, callback)
=> Localize currently lacks a way to register callback functions for lang change notifications. Like said this callback-thingy really needs to be added to Localize, but this should be quite easy to implement.

obs.connect()
=> similar to: connectedElements.set(elem, detectLanguage(elem))

obs.disconnect()
=> similar to: connectedElements.delete(elem)

obs.getLocale()
=> similar to: connectedElements.get(elem)

detectLocale(elem)
=> same as: connectedElements.get(elem) || detectLanguage(elem)
only in the latest version

@xdev1
Copy link

xdev1 commented Nov 18, 2021

@claviska I know, it's currently not important, but just FYI:
As a replacement for my silly proposal above, the request of making the "lang attribute changes" detection completely library independent could easily be achieved with something the following additional 5 lines (the only part that different libraries would have to share is the event name ... and the basic idea, of course):

image

PS: Code has not been tested
[Edit] PPS: Eehm, xdev1 is obviously just a different username for the same person as above...

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