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

Do not store subscribers by function name #156

Open
ostgals opened this issue Oct 22, 2021 · 3 comments
Open

Do not store subscribers by function name #156

ostgals opened this issue Oct 22, 2021 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@ostgals
Copy link
Contributor

ostgals commented Oct 22, 2021

It leads to a weird situation when different closures attach two different subscribers with the same name. In this case, the second attached subscriber replaces the first one leading to unsubscribing in the first closure.

Possible fixes

Just remove the check if the subscriber is named function or store subscribers as function instances.

Possible workaround until the bug is fixed

Pass anonymous subscribers to the .subscribe method like

nb.subscribe(() => update()) // not nb.subscribe(update)
@ostgals ostgals added the bug Something isn't working label Oct 22, 2021
@esneko
Copy link
Member

esneko commented Jul 26, 2022

Please provide an example of client code with use case for the same function subscribed more than once.

@ostgals
Copy link
Contributor Author

ostgals commented Jul 26, 2022

Okay, here is an elementary React example of what I mean:

const List = ({ nexus, onSelectRow }) => {
  const [records, setRecords] = React.useState([])

  React.useEffect(() => {
    const update = () => setRecords(nexus.getRecordSet())

    const token = nexus.subscribe(update) // token === 'update'
    return () => nexus.unsubscribe(token)
  }, [nexus])

  return (/* records rendered as a list with pagination controls */)
}

const Form = ({ nexus, onClose }) => {
  const [model, setModel] = React.useState({})

  React.useEffect(() => {
    const update = ({ controls, methods }) => {
      setModel(nexus.getCurrentRecordModel())
    }

    const token = nexus.subscribe(update) // token === 'update'
    return () => nexus.unsubscribe(token)
  }, [nexus])

  return (/* form that allows to view and edit specific record */)
}

const ViewApp = () => {
  const nexus = NexusFactory('example')

  const [showForm, toggleForm] = React.useState(false)

  const handleSelectRow = index => {
    nexus.positionOnRow(index)
    toggleForm(true)
  }

  return (
    <>
      <List nexus={nexus} onSelectRow={handleSelectRow} />
      {showForm && <Form nexus={nexus} onClose={() => toggleForm(false)} />}
    </>
  )
}

The point is that two different components create two subscriptions to the same Nexus instance. As soon as Form is mounted its subscription replaces the earlier subscription created by List because both components use subscriber functions with the same name update! As a result, the List is never updated anymore.

So storing subscriptions using function names basically is not a good idea.

@esneko
Copy link
Member

esneko commented Jul 27, 2022

Ok, I see now the named function is stored by its name, e.g. update.

this.subscribers.push({ token: fn.name, fn })

Basically we need to remove tokens and store function objects directly:

this.subscribers = [...this.subscribers, fn]

React example from above should be covered in this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants