From d330975557d90591aa69dc6701d5a2917ce77b09 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Wed, 17 Oct 2018 10:06:26 +0200 Subject: [PATCH] fix endless loop, by using the same reference for the branch array --- scm-ui/src/repos/containers/BranchRoot.js | 54 ++++++----------- scm-ui/src/repos/containers/BranchSelector.js | 3 + scm-ui/src/repos/containers/Changesets.js | 16 +++-- scm-ui/src/repos/containers/RepositoryRoot.js | 32 ++++++---- scm-ui/src/repos/modules/branches.js | 58 +++++-------------- scm-ui/src/repos/modules/branches.test.js | 46 +++++++-------- 6 files changed, 90 insertions(+), 119 deletions(-) diff --git a/scm-ui/src/repos/containers/BranchRoot.js b/scm-ui/src/repos/containers/BranchRoot.js index 8b1965044a..1a181ce0b8 100644 --- a/scm-ui/src/repos/containers/BranchRoot.js +++ b/scm-ui/src/repos/containers/BranchRoot.js @@ -18,6 +18,7 @@ import { compose } from "redux"; type Props = { repository: Repository, baseUrl: string, + selected: string, // State props branches: Branch[], @@ -31,37 +32,16 @@ type Props = { match: any }; -type State = { - selectedBranch?: Branch -}; - -class BranchRoot extends React.Component { +class BranchRoot extends React.Component { constructor(props: Props) { super(props); this.state = {}; } componentDidMount() { - console.log("BR did mount"); this.props.fetchBranches(this.props.repository); } - componentDidUpdate(prevProps: Props) { - const { branches, match, loading } = this.props; - console.log("BR did update"); - const branchName = decodeURIComponent(match.params.branch); - if (branches) { - if ( - (!loading && prevProps.loading) || - match.url !== prevProps.match.url - ) { - this.setState({ - selectedBranch: branches.find(b => b.name === branchName) - }); - } - } - } - stripEndingSlash = (url: string) => { if (url.endsWith("/")) { return url.substring(0, url.length - 1); @@ -80,9 +60,14 @@ class BranchRoot extends React.Component { ); }; + findSelectedBranch = () => { + const { selected, branches } = this.props; + return branches.find((branch: Branch) => branch.name === selected); + }; + render() { - console.log("BR render"); - const { repository, match, branches, loading } = this.props; + // TODO error??? + const { repository, loading, match, branches } = this.props; const url = this.stripEndingSlash(match.url); if (loading) { @@ -92,6 +77,9 @@ class BranchRoot extends React.Component { return null; } + const branch = this.findSelectedBranch(); + const changesets = ; + return ( <> { this.branchSelected(b); }} /> - ( - - )} - /> + changesets} /> ); } @@ -123,15 +103,17 @@ const mapDispatchToProps = dispatch => { }; const mapStateToProps = (state: any, ownProps: Props) => { - const { repository } = ownProps; + const { repository, match } = ownProps; const loading = isFetchBranchesPending(state, repository); const error = getFetchBranchesFailure(state, repository); - const branches = getBranches(state, repository); + const selected = decodeURIComponent(match.params.branch); + return { loading, error, - branches + branches, + selected }; }; diff --git a/scm-ui/src/repos/containers/BranchSelector.js b/scm-ui/src/repos/containers/BranchSelector.js index bec66ca143..833ed20ad6 100644 --- a/scm-ui/src/repos/containers/BranchSelector.js +++ b/scm-ui/src/repos/containers/BranchSelector.js @@ -10,13 +10,16 @@ type Props = { }; type State = { selectedBranch?: Branch }; + class BranchSelector extends React.Component { constructor(props: Props) { super(props); this.state = {}; } + render() { const { branches } = this.props; + if (branches) { return ( <> diff --git a/scm-ui/src/repos/containers/Changesets.js b/scm-ui/src/repos/containers/Changesets.js index c88b2d40a1..8df046e21a 100644 --- a/scm-ui/src/repos/containers/Changesets.js +++ b/scm-ui/src/repos/containers/Changesets.js @@ -1,8 +1,13 @@ // @flow import React from "react"; -import {withRouter} from "react-router-dom"; -import type {Branch, Changeset, PagedCollection, Repository} from "@scm-manager/ui-types"; +import { withRouter } from "react-router-dom"; +import type { + Branch, + Changeset, + PagedCollection, + Repository +} from "@scm-manager/ui-types"; import { fetchChangesetsByBranch, fetchChangesetsByBranchAndPage, @@ -11,10 +16,10 @@ import { isFetchChangesetsPending, selectListAsCollection } from "../modules/changesets"; -import {connect} from "react-redux"; +import { connect } from "react-redux"; import ChangesetList from "../components/changesets/ChangesetList"; -import {ErrorPage, LinkPaginator, Loading} from "@scm-manager/ui-components"; -import {translate} from "react-i18next"; +import { ErrorPage, LinkPaginator, Loading } from "@scm-manager/ui-components"; +import { translate } from "react-i18next"; type Props = { repository: Repository, //TODO: Do we really need/want this here? @@ -39,7 +44,6 @@ type State = {}; class Changesets extends React.Component { componentDidMount() { - console.log("CDM"); const { fetchChangesetsByBranch, fetchChangesetsByBranchAndPage, diff --git a/scm-ui/src/repos/containers/RepositoryRoot.js b/scm-ui/src/repos/containers/RepositoryRoot.js index d94d2b855f..416e0544a4 100644 --- a/scm-ui/src/repos/containers/RepositoryRoot.js +++ b/scm-ui/src/repos/containers/RepositoryRoot.js @@ -1,16 +1,29 @@ //@flow import React from "react"; -import {deleteRepo, fetchRepo, getFetchRepoFailure, getRepository, isFetchRepoPending} from "../modules/repos"; -import {connect} from "react-redux"; -import {Route, Switch} from "react-router-dom"; -import type {Repository} from "@scm-manager/ui-types"; -import {ErrorPage, Loading, Navigation, NavLink, Page, Section} from "@scm-manager/ui-components"; -import {translate} from "react-i18next"; +import { + deleteRepo, + fetchRepo, + getFetchRepoFailure, + getRepository, + isFetchRepoPending +} from "../modules/repos"; +import { connect } from "react-redux"; +import { Route, Switch } from "react-router-dom"; +import type { Repository } from "@scm-manager/ui-types"; +import { + ErrorPage, + Loading, + Navigation, + NavLink, + Page, + Section +} from "@scm-manager/ui-components"; +import { translate } from "react-i18next"; import RepositoryDetails from "../components/RepositoryDetails"; import DeleteNavAction from "../components/DeleteNavAction"; import Edit from "../containers/Edit"; -import type {History} from "history"; +import type { History } from "history"; import EditNavLink from "../components/EditNavLink"; import BranchRoot from "./BranchRoot"; @@ -98,9 +111,8 @@ class RepositoryRoot extends React.Component { path={`${url}/edit`} component={() => } /> - ( { diff --git a/scm-ui/src/repos/modules/branches.js b/scm-ui/src/repos/modules/branches.js index 2a4580e194..3148857f51 100644 --- a/scm-ui/src/repos/modules/branches.js +++ b/scm-ui/src/repos/modules/branches.js @@ -66,79 +66,51 @@ export function fetchBranchesFailure(repository: Repository, error: Error) { // Reducers +type State = { [string]: Branch[] }; + export default function reducer( - state: Object = {}, + state: State = {}, action: Action = { type: "UNKNOWN" } -): Object { +): State { switch (action.type) { case FETCH_BRANCHES_SUCCESS: const key = createKey(action.payload.repository); - let oldBranchesByNames = { [key]: {} }; - if (state[key] !== undefined) { - oldBranchesByNames[key] = state[key]; - } return { - [key]: { - byNames: extractBranchesByNames( - action.payload.data, - oldBranchesByNames[key].byNames - ) - } + ...state, + [key]: extractBranchesFromPayload(action.payload.data) }; default: return state; } } -function extractBranchesByNames(data: any, oldBranchesByNames: any): ?Object { - if (!data._embedded || !data._embedded.branches) { - return {}; +function extractBranchesFromPayload(payload: any) { + if (payload._embedded && payload._embedded.branches) { + return payload._embedded.branches; } - const branches = data._embedded.branches; - const branchesByNames = {}; - - for (let branch of branches) { - branchesByNames[branch.name] = branch; - } - - for (let name in oldBranchesByNames) { - branchesByNames[name] = oldBranchesByNames[name]; - } - return branchesByNames; + return []; } // Selectors -export function getBranchNames( - state: Object, - repository: Repository -): ?Array { - const key = createKey(repository); - if (!state.branches || !state.branches[key] || !state.branches[key].byNames) { - return []; - } - return Object.keys(state.branches[key].byNames); -} +const empty = []; export function getBranches(state: Object, repository: Repository) { const key = createKey(repository); if (state.branches[key]) { - if (state.branches[key].byNames) { - return Object.values(state.branches[key].byNames); - } + return state.branches[key]; } + return empty; } export function getBranch( - state: Object, + state: State, repository: Repository, name: string ): ?Branch { const key = createKey(repository); if (state.branches[key]) { - if (state.branches[key].byNames[name]) { - return state.branches[key].byNames[name]; - } + return state.branches[key].find((b: Branch) => b.name === name); } return null; } diff --git a/scm-ui/src/repos/modules/branches.test.js b/scm-ui/src/repos/modules/branches.test.js index c958f105ef..40abf951cf 100644 --- a/scm-ui/src/repos/modules/branches.test.js +++ b/scm-ui/src/repos/modules/branches.test.js @@ -109,23 +109,20 @@ describe("branches", () => { const newState = reducer({}, action); expect(newState).toBeDefined(); expect(newState[key]).toBeDefined(); - expect(newState[key].byNames["branch1"]).toEqual(branch1); - expect(newState[key].byNames["branch2"]).toEqual(branch2); + expect(newState[key]).toContain(branch1); + expect(newState[key]).toContain(branch2); }); it("should not delete existing branches from state", () => { const oldState = { - "foo/bar": { - byNames: { - branch3: branch3 - } - } + "hitchhiker/heartOfGold": [branch3] }; const newState = reducer(oldState, action); - expect(newState[key].byNames["branch1"]).toEqual(branch1); - expect(newState[key].byNames["branch2"]).toEqual(branch2); - expect(newState[key].byNames["branch3"]).toEqual(branch3); + expect(newState[key]).toContain(branch1); + expect(newState[key]).toContain(branch2); + + expect(newState["hitchhiker/heartOfGold"]).toContain(branch3); }); }); @@ -134,12 +131,7 @@ describe("branches", () => { const state = { branches: { - [key]: { - byNames: { - branch1, - branch2 - } - } + [key]: [branch1, branch2] } }; @@ -153,13 +145,6 @@ describe("branches", () => { expect(isFetchBranchesPending(state, repository)).toBeTruthy(); }); - it("should return branches names", () => { - const names = getBranchNames(state, repository); - expect(names.length).toEqual(2); - expect(names).toContain("branch1"); - expect(names).toContain("branch2"); - }); - it("should return branches", () => { const branches = getBranches(state, repository); expect(branches.length).toEqual(2); @@ -167,15 +152,28 @@ describe("branches", () => { expect(branches).toContain(branch2); }); + it("should return always the same reference for branches", () => { + const one = getBranches(state, repository); + const two = getBranches(state, repository); + expect(one).toBe(two); + }); + it("should return single branch by name", () => { const branch = getBranch(state, repository, "branch1"); expect(branch).toEqual(branch1); }); + it("should return same reference for single branch by name", () => { + const one = getBranch(state, repository, "branch1"); + const two = getBranch(state, repository, "branch1"); + expect(one).toBe(two); + }); + it("should return undefined if branch does not exist", () => { const branch = getBranch(state, repository, "branch42"); - expect(branch).toBeFalsy(); + expect(branch).toBeUndefined(); }); + it("should return error if fetching branches failed", () => { const state = { failure: {