Page MenuHomePhabricator
Differential D721

Don't use a custom keyMapper
ClosedPublic
Actions

Authored by max on Jul 28 2019, 9:08 PM.

Details

Reviewers
schrockn
bengotow
Group Reviewers
Restricted Project
Commits
R1:b568e44bed11: Don't use a custom keyMapper
Summary

Fixes https://github.com/dagster-io/dagster/issues/1617, with a potential hit to render performance. See: https://github.com/bvaughn/react-virtualized/blob/master/docs/CellMeasurer.md#prop-types-1

Test Plan

Manual

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

max created this revision.Jul 28 2019, 9:08 PM
Harbormaster completed remote builds in B2613: Diff 3266.Jul 28 2019, 9:17 PM
schrockn added a comment.Jul 29 2019, 2:57 AM

How did you verify that this is a perf win?

max added a comment.Jul 29 2019, 3:00 AM

The opposite, I think this might make table render performance worse; I haven't measured.

schrockn added a comment.Jul 29 2019, 3:02 AM

ah i misread

schrockn accepted this revision.Jul 29 2019, 3:12 AM
This revision is now accepted and ready to land.Jul 29 2019, 3:12 AM
schrockn requested changes to this revision.Jul 29 2019, 3:16 AM

Filtering breaks this apparently

This revision now requires changes to proceed.Jul 29 2019, 3:16 AM
max added a comment.Jul 29 2019, 3:34 AM

This is broken in Chrome

max added a comment.Jul 29 2019, 3:37 AM

and on FIrefox

max updated this revision to Diff 3276.Jul 29 2019, 3:49 AM

Resolve issue with filtering nodes

Harbormaster completed remote builds in B2619: Diff 3276.Jul 29 2019, 3:58 AM
schrockn accepted this revision.Jul 29 2019, 4:25 AM

verified fix locally

This revision is now accepted and ready to land.Jul 29 2019, 4:25 AM
bengotow accepted this revision.Jul 29 2019, 3:38 PM

Looks good to me! I think since we're blowing away and re-creating the cache now when the node set changes, we don't need the custom key mapping (you're correct that it was allowing you to filter the list without recomputing the height of the rows).

Closed by commit R1:b568e44bed11: Don't use a custom keyMapper (authored by max). · Explain WhyJul 29 2019, 3:43 PM
This revision was automatically updated to reflect the committed changes.
Max Gasner <max.gasner@gmail.com> added a commit: R1:b568e44bed11: Don't use a custom keyMapper.

Revision Contents
Changeset List

PathSize
js_modules/
dagit/
src/
runs/
42 lines
16 lines

Diff 3277

View Options

js_modules/dagit/src/runs/LogsRow.tsx

Loading...
View Options

js_modules/dagit/src/runs/LogsScrollingTable.tsx

Loading...