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

Prvent early _debounceScrollEndedCallback(&cellRangeRenderer), when process slow Grid's #1141

Merged
merged 1 commit into from
Jul 11, 2018

Conversation

Gvozd
Copy link
Contributor

@Gvozd Gvozd commented Jun 17, 2018

When I render Grid with big count of visible cells, and/or slow renderCell, scrolling so slow, beacause _debounceScrollEndedCallback force additional renders
At now _debounceScrollEndedCallback really called at end of scrolling

  • The existing test suites (npm test) all pass
  • For any new features or bug fixes, both positive and negative test cases have been added
  • For any new features, documentation has been added
    Not relevant for this.
  • For any documentation changes, the text has been proofread and is clear to both experienced users and beginners.
    Not relevant for this.
  • Format your code with prettier (npm run prettier).
    npm run prettier failed by No matching files., but eslint succeed
  • Run the Flow typechecks (npm run typecheck).

@Gvozd
Copy link
Contributor Author

Gvozd commented Jul 8, 2018

BUMP
Neede review and merge

Copy link
Collaborator

@wuweiweiwu wuweiweiwu left a comment

Choose a reason for hiding this comment

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

Do you mind explaining how Promise.resolve().then waits for the end of processing of the current event handler?

@Gvozd
Copy link
Contributor Author

Gvozd commented Jul 8, 2018

Promis.resolve planned as microtask
https://jakearchibald.com/2015/tasks-microtasks-queues-and-schedules/

Simple example, and his timeline

window.onclick = function() {
	sync('start');
	// 1 - plan promise, and timer
	Promise.resolve().then(function() {
		// 2 - promise resolved at microtask(at end of click)
		sync('promise');
	});
	setTimeout(function() {
		// 3 - timer triggered after click event(his handler, and microtask)
		sync('timeout');
	});
	sync('end');
}

function sync(msg) {
	var start = Date.now();
	while(Date.now() - start < 1000);
	console.log(`sync ${msg}`);
}

image

@wuweiweiwu
Copy link
Collaborator

@Gvozd Thanks for the link! I'll give that a look

Copy link
Collaborator

@wuweiweiwu wuweiweiwu left a comment

Choose a reason for hiding this comment

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

After minor comment this looks good!

TIL about the browser event loop. Thanks @Gvozd :)

cellRangeRendererCalls.push(props);
return defaultCellRangeRenderer(props);
}
const props = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe the only prop you need to override from the default is scrollingResetTimeInterval

getMarkup https://github.com/bvaughn/react-virtualized/blob/master/source/Grid/Grid.jest.js#L44 provides the required default props which shouldn't additionally specified unless necessary for the test.

@Gvozd
Copy link
Contributor Author

Gvozd commented Jul 9, 2018

Fix test by CodeReview
Branch rebased, and squashed

Copy link
Collaborator

@wuweiweiwu wuweiweiwu left a comment

Choose a reason for hiding this comment

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

Nice! Thanks

@wuweiweiwu wuweiweiwu merged commit 42c8fbc into bvaughn:master Jul 11, 2018
errendir added a commit to IdeaFlowCo/react-virtualized that referenced this pull request Oct 24, 2018
* bvaughn/master: (54 commits)
  Update version and changelog for 9.21.0 release (bvaughn#1252)
  chore: update lockfile
  Update ci badge (bvaughn#1227)
  Allow users to override default table row styles (bvaughn#1175)
  Add onColumnClick to Table (bvaughn#1207)
  remove unused variable in Masonry.example.js (bvaughn#1218)
  Fix Table aria attributes (bvaughn#1208)
  Fix typo in CellMeasurer.DynamicHeightTableColumn.example.js (bvaughn#1190)
  Update usingAutoSizer.md (bvaughn#1186)
  Add an extra check for an e.target.className.indexOf function (bvaughn#1210)
  Fix broken Slack badge image (bvaughn#1205)
  docs(CellMeasurer): fix `import` statement (bvaughn#1187)
  Added new friend (bvaughn#1197)
  Fix createMultiSort bug (bvaughn#1051)
  adding new usecase example and fix some typos (bvaughn#1168)
  Updating version to 9.20.1
  Update changelog for the 9.20.1 release (bvaughn#1167)
  Prevent early debounceScrollEndedCallback when there is a slow render (bvaughn#1141)
  removing sideEffects (bvaughn#1163)
  fix for bvaughn#998 with test cases (bvaughn#1154)
  ...
const start = Date.now();
let start;
// wait for end of processing current event handler, because event handler may be long
Promise.resolve().then(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI this adds core-js polyfill to the output

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea what to do with it.
Looks like we'll have to leave it at that.

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

3 participants