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

Upgrade react-day-picker to v7 #2055

Merged
merged 8 commits into from
Feb 1, 2018
Merged

Upgrade react-day-picker to v7 #2055

merged 8 commits into from
Feb 1, 2018

Conversation

skovy
Copy link
Contributor

@skovy skovy commented Jan 30, 2018

Fixes #2054

Changes proposed in this pull request:

Upgrade the react-day-picker dependency from version ^5.3.0 to ^7.0.7 and update to accomadate breaking changes in the react-day-picker package highlighted in their CHANGELOG.

Reviewers should focus on:

Type accuracy and breaking changes highlighted in the react-day-picker CHANGELOG.

@palantirtech
Copy link
Member

Thanks for your interest in palantir/blueprint, @skovy! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@skovy
Copy link
Contributor Author

skovy commented Jan 30, 2018

Getting issues using submodule exports. Not sure the best steps to resolve, because it looks like react-day-picker does not export DayPickerProps

@blueprintjs/datetime: Submodule import paths from this package are disallowed; import from the root instead (no-submodule-imports)
@blueprintjs/datetime:    8 | import * as moment from "moment";
@blueprintjs/datetime:    9 | import * as React from "react";
@blueprintjs/datetime: > 10 | import { DayPickerProps } from "react-day-picker/types/props";
  1. Can I disable the rule for these lines using a disable comment?
  2. Open a PR for react-day-picker to add these to the index export?

@adidahiya adidahiya added this to the 2.0.0 milestone Jan 30, 2018
@adidahiya
Copy link
Contributor

@skovy you can add an exception to this list in the tslint config:

"@blueprintjs/test-commons/bootstrap",

add "react-day-picker/types" to the array.

@@ -7,7 +7,7 @@
import * as classNames from "classnames";
import * as moment from "moment";
import * as React from "react";
import * as ReactDayPicker from "react-day-picker";
import { DayPickerProps } from "react-day-picker/types/props";
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to import this without a submodule? like from react-day-picker itself like it used to be? (i haven't poked at the actual package yet)

Copy link
Contributor

@giladgray giladgray Jan 31, 2018

Choose a reason for hiding this comment

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

on further examination, it seems not. index.d.ts only exports DayPicker 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

in this case, @skovy please add the tslint-config exception as suggested by @adidahiya above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 yea, I was looking into that as well and was disappointed only DayPicker was exported.

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -7,7 +7,7 @@
import * as classNames from "classnames";
import * as moment from "moment";
import * as React from "react";
import * as ReactDayPicker from "react-day-picker";
import { DayPickerProps } from "react-day-picker/types/props";
Copy link
Contributor

Choose a reason for hiding this comment

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

in this case, @skovy please add the tslint-config exception as suggested by @adidahiya above.

@giladgray
Copy link
Contributor

@skovy nice! tests failures seem to be relevant -- DayPicker-related. check 'em out 🔨

@skovy
Copy link
Contributor Author

skovy commented Jan 31, 2018

@giladgray yea noticed that. I can dig into that, definitely seems related to the breaking changes in the CHANGELOG, just trying to understand the specs, abstractions and how to debug 😋

Any useful docs in the repo for good debugging techniques?

@giladgray
Copy link
Contributor

yarn test:karma:debug in the datetime package will run the tests in watch mode.

And yarn dev:datetime in the Root directory will launch the dev server of the docs site which reloads on changes.

Happy hunting!

locale,
localeUtils,
modifiers,
showOutsideDays: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Curious if this prop should be configurable given it's always hidden since this PR https://github.com/palantir/blueprint/pull/586/files#r98813760?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can leave this as is for now. Seems like changing it here would be out of scope for this PR.

@@ -14,6 +14,10 @@
margin-right: $pt-grid-size;
}

.DayPicker-NavButton--interactionDisabled {
display: none;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Breaking change in v6.0.0. The element is now always rendered, but with this CSS class conditionally: https://github.com/gpbl/react-day-picker/blob/master/CHANGELOG.md#v600-2017-06-16

assert.lengthOf(document.getElementsByClassName("DayPicker-NavButton--next"), 0);
// react-day-picker still renders the navigation but with a interaction disabled class
assert.lengthOf(document.getElementsByClassName("DayPicker-NavButton--prev"), 1);
assert.lengthOf(document.getElementsByClassName("DayPicker-NavButton--next"), 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if there is a better way to actually assert visibility of these elements, given they are now always rendered.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to reach into the style of each element to check the visibility?

Copy link
Contributor

Choose a reason for hiding this comment

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

what about looking for that conditional class?

Copy link
Contributor

Choose a reason for hiding this comment

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

or, if they're always there, just remove these assertions. no need to assert on external library behavior. case in point: when that behavior changes and breaks your tests (but our code is still fine).

Copy link
Contributor

@cmslewis cmslewis left a comment

Choose a reason for hiding this comment

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

Looks okay to me! Left a couple requests, but nothing blocking. nit: I mildly prefer keeping the namespacing for the ReactDayPicker types if possible, but I'm okay either way.

I filed an issue in the react-day-picker requesting that all types be exported properly: gpbl/react-day-picker#617.

@@ -7,7 +7,7 @@
import * as classNames from "classnames";
import * as moment from "moment";
import * as React from "react";
import * as ReactDayPicker from "react-day-picker";
import { DayPickerProps } from "react-day-picker/types/props";
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -7,7 +7,9 @@
import { AbstractPureComponent, Button, IProps, Utils } from "@blueprintjs/core";
import * as classNames from "classnames";
import * as React from "react";
import * as ReactDayPicker from "react-day-picker";
import { default as ReactDayPicker } from "react-day-picker";
Copy link
Contributor

Choose a reason for hiding this comment

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

Whoa, funky syntax. Haven't seen this before, but it's interesting. If it's equivalent, should we just do:

import ReactDayPicker from "react-day-picker";

?

Source: https://stackoverflow.com/a/31092287/5199574

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would have to be import DayPicker from "react-day-picker";, since the default export is DayPicker (not ReactDayPicker). However, that conflicts with the actual DayPicker component name. So used this approach to rename DayPicker to ReactDayPicker. Open to other alternatives, but as far as I know, the above recommendation won't work?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would have to be import DayPicker from "react-day-picker";, since the default export is DayPicker (not ReactDayPicker)

Are you sure about this? default exports are (by design) anonymous, so you can import them with whatever name you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

it will. default exports don't have a name of their own. you can name them whatever you want with the import DefaultName from "package" syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow. I swear I tried that 🤔 TIL. Appreciate the feedback! 😊

locale,
localeUtils,
modifiers,
showOutsideDays: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

We can leave this as is for now. Seems like changing it here would be out of scope for this PR.

assert.lengthOf(document.getElementsByClassName("DayPicker-NavButton--next"), 0);
// react-day-picker still renders the navigation but with a interaction disabled class
assert.lengthOf(document.getElementsByClassName("DayPicker-NavButton--prev"), 1);
assert.lengthOf(document.getElementsByClassName("DayPicker-NavButton--next"), 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to reach into the style of each element to check the visibility?

@giladgray
Copy link
Contributor

@skovy a few small comments still need to be addressed (esp. the default import thing), then this will be good to go!

@skovy
Copy link
Contributor Author

skovy commented Feb 1, 2018

@giladgray appreciate all the feedback. Updated the default imports and removed some of the redundant assertions in the specs. Please take another look 👍

@skovy
Copy link
Contributor Author

skovy commented Feb 1, 2018

Not sure if this is related or a flake?

FAILED TESTS:
    ✖ "after each" hook
      PhantomJS 2.1.1 (Linux 0.0.0)
    TypeError: undefined is not an object (evaluating 'wrapper.assertIsOpen') (test/index.ts:102819)

@giladgray
Copy link
Contributor

@skovy when was the last time you merged develop into this branch? (our latest develop, not your fork's.) done a lot of work this week to address failures like these.

Copy link
Contributor

@giladgray giladgray left a comment

Choose a reason for hiding this comment

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

confirmed that tests are fixed after merging develop locally, so I'm just gonna merge this guy.

thank you @skovy for a PR well done! 👏

@giladgray giladgray merged commit dde2d32 into palantir:develop Feb 1, 2018
@skovy skovy deleted the upgrade-react-day-picker-v7 branch February 1, 2018 23:11
@skovy
Copy link
Contributor Author

skovy commented Feb 4, 2018

Hey all, curious when the next beta will be released containing this PR?

@giladgray
Copy link
Contributor

@skovy tomorrow!

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.

5 participants