[Table] Validation improvements #1587

cmslewis · 2017-09-20T23:27:03Z

Fixes #1371

Checklist

Include tests

Changes proposed in this pull request:

🐛 FIXED Table now validates props on mount and on update.
🔄 CHANGED Table now validates props as follows (assuming each prop involved is defined):
- Errors:
  - numRows < 0
  - numFrozenRows < 0
  - numFrozenColumns < 0
  - numRows !== rowHeights.length
  - numColumns !== columnWidths.length
  - non-<Column> child (*)
- Warnings:
  - numFrozenRows > numRows (changed the message to mention that we'll clamp for you)
  - numFrozenColumns > numColumns (same)

Reviewers should focus on:

How does the error-message language look?
(*) Is this the right choice? Per my code comment, we're currently just printing a warning, but the Table would eventually throw a convoluted error from deep inside when processing non-<Column> children, so I figure we might as well just throw a clear error at the outset.

Screenshot

Validation
  on mount
    errors
      ✔ throws an error if numRows < 0
      ✔ throws an error if numFrozenRows < 0
      ✔ throws an error if numFrozenColumns < 0
      ✔ throws an error if rowHeights.length !== numRows
      ✔ throws an error if columnWidths.length !== number of <Column>s
      ✔ throws an error if a non-<Column> child is provided
    warnings
      ✔ should print a warning when numFrozenRows > numRows
      ✔ should print a warning when numFrozenColumns > num <Column>s
  on update
    errors
      ✔ on numRows update, throws an error if numRows < 0
      ✔ on numFrozenRows update, throws an error if numFrozenRows < 0
      ✔ on numFrozenColumns update, throws an error if numFrozenColumns < 0
      ✔ on numRows update, throws an error if rowHeights.length !== numRows
      ✔ on rowHeights update, throws an error if rowHeights.length !== numRows
      ✔ on children update, throws an error if columnWidths.length !== number of <Column>s
      ✔ on columnWidths update, throws an error if columnWidths.length !== number of <Column>s
      ✔ on children update, throws an error if a non-<Column> child is provided
    warnings
      ✔ on numFrozenRows update, should print a warning when numFrozenRows > numRows
      ✔ on numRows update, should print a warning when numFrozenRows > numRows
      ✔ on numFrozenColumns update, should print a warning when numFrozenColumns > num <Column>s
      ✔ on children update, should print a warning when numFrozenColumns > num <Column>s
      ✔ should print a warning when numFrozenColumns > num <Column>s

cmslewis

Quick thoughts/questions.

cmslewis · 2017-09-20T23:27:27Z

packages/table/src/table.tsx

@@ -524,6 +524,8 @@ export class Table extends AbstractComponent<ITableProps, ITableState> {
    }

    public componentWillReceiveProps(nextProps: ITableProps) {
+        super.componentWillReceiveProps(nextProps);


cmslewis · 2017-09-20T23:28:13Z

packages/table/src/table.tsx

+            // the second part of this conditional will never be true, but it
+            // informs the TS compiler that we won't be invoking
+            // childType.prototype on a "string" element.
+            if (typeof child === "string" || typeof childType === "string") {


Janky, but it was throwing an error before I added the typeof child === "string" check. Turns out typeof childType === undefined for strings, so the check wasn't working as expected before.

...so it's a... typeguard? That's... weird. I am confused as to how this could possibly be working. But... ok?

typeof childType === "string" is the typeguard, yeah. But it never returns true. @giladgray, do you know a way out of this comical quagmire?

cmslewis · 2017-09-20T23:29:04Z

packages/table/src/table.tsx

            } else {
                const isColumn = childType.prototype === Column.prototype || Column.prototype.isPrototypeOf(childType);
                if (!isColumn) {
-                    console.warn(Errors.TABLE_NON_COLUMN_CHILDREN_WARNING);
+                    throw new Error(Errors.TABLE_NON_COLUMN_CHILDREN_WARNING);


Is there ever a case where we'd want to let people wrap child <Column>s with a thing? If not, would prefer this to be an Error.

I think you're probably right, @adidahiya might know if the other ever comes up? Not sure.

do we really need to validate the react children? what does this buy us? what's the harm in being lenient? please keep #1108 in mind; enforcing constraints on React children in typescript usually gets in the way more often than it helps.

table.tsx accesses props.children expecting it to be an array of <Column>s (e.g. in Table.getColumnProps). If that accessing fails, the error message is somewhat convoluted; throwing a clear error from validateProps would be more helpful.

Only reservation is that maybe there are cases when people would want to compose a basic <Column> into something else they'd want to provide as a direct child of Table? In that case, a duck-typing approach would seem preferable (i.e. as long as the child in question supports the Column API, we don't care).

I've never heard of anyone doing the above customization, so I'm including the error check as a result of my first point. ^ Thoughts?

for now I'm fine with enforcing Column to be a direct child of Table -- as you said we haven't seen the wrapping component use case come up. makes sense to have a better error message and halt execution since the table won't render correctly. let's get to #1108 soon for 2.0.

blueprint-bot · 2017-09-20T23:35:36Z

Move warnings to bottom

Preview: documentation | table
Coverage: core | datetime

gscshoyru · 2017-09-21T16:50:45Z

packages/table/src/table.tsx

@@ -524,6 +524,8 @@ export class Table extends AbstractComponent<ITableProps, ITableState> {
    }

    public componentWillReceiveProps(nextProps: ITableProps) {
+        super.componentWillReceiveProps(nextProps);


gscshoyru · 2017-09-21T16:58:58Z

packages/table/src/table.tsx

+            // the second part of this conditional will never be true, but it
+            // informs the TS compiler that we won't be invoking
+            // childType.prototype on a "string" element.
+            if (typeof child === "string" || typeof childType === "string") {


...so it's a... typeguard? That's... weird. I am confused as to how this could possibly be working. But... ok?

gscshoyru · 2017-09-21T16:59:48Z

packages/table/src/table.tsx

            } else {
                const isColumn = childType.prototype === Column.prototype || Column.prototype.isPrototypeOf(childType);
                if (!isColumn) {
-                    console.warn(Errors.TABLE_NON_COLUMN_CHILDREN_WARNING);
+                    throw new Error(Errors.TABLE_NON_COLUMN_CHILDREN_WARNING);


I think you're probably right, @adidahiya might know if the other ever comes up? Not sure.

cmslewis · 2017-09-21T18:13:13Z

Adding @adidahiya for CR because you've used the Table in the wild.

cmslewis added 11 commits September 20, 2017 14:41

Throw error when numRows != rowHeights.length

1b4d2a6

Throw error when num columns !== columnWidths.length

3058d6f

Do cheap checks first

404917e

Add unit tests + bug fix

c92c629

Add validation + tests for numRows < 0

28ecea7

Throw errors when numFrozen{Rows,Columns} < 0 (for consistency)

b088017

Reorder checks in validateProps

a57b5df

Bucket tests into 'errors' and 'warnings' blocks

2eef44e

Add tests for warnings

9e10dc7

Throw error when children aren't <Column>s + other cleanups

de14212

Move warnings to bottom

4c0c24d

cmslewis added the Package: table label Sep 20, 2017

cmslewis requested review from llorca, giladgray and gscshoyru September 20, 2017 23:27

cmslewis commented Sep 20, 2017

View reviewed changes

gscshoyru approved these changes Sep 21, 2017

View reviewed changes

cmslewis requested a review from adidahiya September 21, 2017 18:12

llorca approved these changes Sep 21, 2017

View reviewed changes

cmslewis merged commit d553cd2 into master Sep 21, 2017

cmslewis deleted the cl/table-1371-row-heights-length branch September 21, 2017 22:49

llorca mentioned this pull request Sep 22, 2017

Prepare release v1.29.0 #1602

Merged

mrshll mentioned this pull request Feb 7, 2018

table does not work with react-hot-loader #723

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Table] Validation improvements #1587

[Table] Validation improvements #1587

cmslewis commented Sep 20, 2017 •

edited

Loading

cmslewis left a comment

cmslewis Sep 20, 2017

gscshoyru Sep 21, 2017

cmslewis Sep 20, 2017

gscshoyru Sep 21, 2017

cmslewis Sep 21, 2017

cmslewis Sep 20, 2017

gscshoyru Sep 21, 2017

adidahiya Sep 21, 2017

cmslewis Sep 21, 2017

adidahiya Sep 21, 2017

cmslewis Sep 21, 2017

blueprint-bot commented Sep 20, 2017

gscshoyru Sep 21, 2017

gscshoyru Sep 21, 2017

gscshoyru Sep 21, 2017

cmslewis commented Sep 21, 2017

[Table] Validation improvements #1587

[Table] Validation improvements #1587

Conversation

cmslewis commented Sep 20, 2017 • edited Loading

Fixes #1371

Checklist

Changes proposed in this pull request:

Reviewers should focus on:

Screenshot

cmslewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Sep 20, 2017

Move warnings to bottom

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmslewis commented Sep 21, 2017

cmslewis commented Sep 20, 2017 •

edited

Loading