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

CRUD refactoring #691

Merged
merged 5 commits into from Dec 20, 2016
Merged

CRUD refactoring #691

merged 5 commits into from Dec 20, 2016

Conversation

pzrq
Copy link
Contributor

@pzrq pzrq commented Dec 15, 2016

While in the area trying to understand how to resolve COMPASS-541, noticed these things I think can be safely removed, or typos fixed.

@pzrq pzrq force-pushed the crud-refactor branch 2 times, most recently from df78275 to 8b6b535 Compare December 15, 2016 07:28
}

// at this point compare the documents themselves and update if not different
return !_.isEqual(nextState.docs, this.state.docs);
Copy link
Member

Choose a reason for hiding this comment

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

This was probably more expensive than React's internal check - did you notice a speed improvement in the list once removing?

@pzrq
Copy link
Contributor Author

pzrq commented Dec 16, 2016

@durran Dug deeper and it was more that the check didn't seem to be doing anything that made Compass more or less responsive, i.e. there doesn't seem to be much value to this code apart from being a source of bugs in the past (this screenshot was from master, not crud-refactor):

screen shot 2016-12-16 at 11 47 20 am

@pzrq pzrq force-pushed the crud-refactor branch 2 times, most recently from 968b3af to 1f5d263 Compare December 16, 2016 12:42
@pzrq
Copy link
Contributor Author

pzrq commented Dec 16, 2016

@durran If you know why these TravisCI timeouts are occurring, fixes welcome 👍

This refactoring is not currently blocking anything (I thought it might block COMPASS-541 but it ultimately turned out to be not required).

I'll most likely wait until your presentation next Tuesday Jaguar time so we can all benefit from your knowledge writing and debugging these tests, i.e. happy to let these build logs stick around until we have a good understanding of their cause and resolution, or at least how to make the errors more deterministic.

@pzrq
Copy link
Contributor Author

pzrq commented Dec 19, 2016

Build failure on master (https://travis-ci.com/10gen/compass/jobs/59828761, which I will restart momentarily) is very similar to https://travis-ci.com/10gen/compass/jobs/59723655

screen shot 2016-12-19 at 11 35 28 am

screen shot 2016-12-19 at 11 35 38 am

@durran
Copy link
Member

durran commented Dec 19, 2016

Note that the "element is not clickable" error is most likely caused by the status bar not finishing its progress and the transparent overlay is still over the entire screen. Another waitForStatusBar here should probably resolve that.

@pzrq
Copy link
Contributor Author

pzrq commented Dec 19, 2016

Yes, good to see element is not clickable fix in master now: f082d1e

Have rebased, hopefully we won't see any more Travis issues, but if we do we'll still learn something.

It was used originally for minicharts/refine bar, see:
c3c2e7a
_key() always returns a new uuid.v4()
Even React recommends against it:
https://facebook.github.io/react/docs/react-component.html#shouldcomponentupdate

I could find no detectable performance or snappiness difference between this version and the version before, using every local dataset I have excluding the perf.big and perf.medium ones (COMPASS-560), such as the mongodb.fanclub, enron.messages and blog.posts collections which are sluggish on initial load but fine afterwards, e.g. when inserting, editing, cloning or a document.
Appears to be completely replaced by the QueryStore in 'compass/src/internal-packages/query/lib/store/query-store.js'
@durran durran merged commit e8cfb38 into master Dec 20, 2016
@durran durran deleted the crud-refactor branch December 20, 2016 00:21
@pzrq
Copy link
Contributor Author

pzrq commented Dec 20, 2016

Thanks @durran 👍

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.

None yet

2 participants