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

Immutable chart state #42

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Immutable chart state #42

wants to merge 2 commits into from

Conversation

djhahe
Copy link

@djhahe djhahe commented Aug 27, 2019

Fixes #31

@djhahe djhahe marked this pull request as ready for review August 27, 2019 11:58
Copy link

@phlickey phlickey left a comment

Choose a reason for hiding this comment

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

Found a few places where the chart object is being mutated.

Would it be easier to just deep copy it at the start of every action, or are there performance concerns?

}

export const onLinkComplete: IOnLinkComplete = (props) => {
const { linkId, fromNodeId, fromPortId, toNodeId, toPortId, config = {} } = props
return (chart: IChart): IChart => {
const links = {...chart.links};
if ((config.validateLink ? config.validateLink({ ...props, chart }) : true) && [fromNodeId, fromPortId].join() !== [toNodeId, toPortId].join()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

To resolve the merge conflict, this condition needs to be updated so that it starts with !config.readonly:

    if (!config.readonly && (config.validateLink ? config.validateLink({ ...props, chart }) : true) && [fromNodeId, fromPortId].join() !== [toNodeId, toPortId].join()) {

@fenech
Copy link
Contributor

fenech commented Oct 22, 2019

It would be great to get this merged, as it would mean that the library could be used with hooks inside functional components!

function useChart(initialState: IChart): [IChart, typeof actions] {
  const [chart, setChart] = useState(initialState);
  const stateActions = useMemo(
    () =>
      Object.entries(actions).reduce(
        (prev, [key, val]) => ({
          ...prev,
          [key]: (...args: any) => setChart(val(...args))
        }),
        {}
      ) as typeof actions,
    [actions]
  );
  return [chart, stateActions];
}

@marcoalfonso
Copy link

Is this pr being merged any time soon?

Have found some bugs on the validateLink function that are caused by this. The link.to property gets wiped when the function is called.

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.

Discussion: Immutable chart actions
4 participants