Skip to content

Conversation

@sjoerdbeentjes
Copy link
Collaborator

When I use the component in an application where it's used in multiple pages, i get this error:

image

It's kind of critical because the component does not function as expected after that.

This happens even if I force rerender the component. It seems like the selectedIds is not cleared when a new instance of the component is rerenderd.

My solution is to clear it manually on every mount.

Copy link
Collaborator

@bob-voorhoede bob-voorhoede left a comment

Choose a reason for hiding this comment

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

I think this can be done neater, by having useSelected return a scoped piece of state and not a global piece of state. Please discuss with @petergoes if this is desirable.

const { activeLegend, setActiveLegend } = useLegend(selectedIds)
const { onSortingChange } = useSortable(layers, root, openItems)
onMounted(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You're delegating responsibility for clearing state (the ref in useSelected) to the Vue component. Is this really desired, or should useSelected() return a 'scoped' piece of state, so that every LayerListControls component has its own selectedIds that is strictly private to that instance of the component?

@petergoes
Copy link
Collaborator

I agree with @bob-voorhoede that it can be done neater.

I looked at useSelected and there the state is created outside of the default function. This leads to the state being created globally which is, imho, the bug. By rewriting useSelected like so:

import { ref } from '@vue/composition-api'

export default function useSelected() {
  const selectedIds = ref([])

  function setSelectedIds(newList) {
    selectedIds.value = newList
  }


  return {
    selectedIds,
    setSelectedIds
  }
}

The selectedIds should be scoped to the component. I think that fixes the problem, but I did not tested it.

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

Successfully merging this pull request may close these issues.

4 participants