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

🐛 Make sure rrule code is included in amp-date-picker #23113

Merged
merged 6 commits into from Jul 1, 2019
Merged

🐛 Make sure rrule code is included in amp-date-picker #23113

merged 6 commits into from Jul 1, 2019

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Jun 28, 2019

In #22946, we switched rrule from a copy in third_party to an installed module. However, we neglected to add an entry for the module in the directories included in the runtime.

This PR fixes things so that the rrule code is included in dist/v0/amp-date-picker-0.1.js.

It will likely have to be cherry-picked to canary before next week's prod push. Edit: None of the currently released versions are affected by this issue. This fix will make it into the next canary and prod releases.

@rsimha rsimha self-assigned this Jun 28, 2019
@rsimha
Copy link
Contributor Author

rsimha commented Jun 28, 2019

@jridgewell @cvializ I had to skip one unit test that appears to be failing now that rrule code is correctly included in amp-date-picker.

Meanwhile, I've confirmed that the rrule code is now included in dist/v0/amp-date-picker-0.1.js, so we should probably merge this for now.

@cvializ
Copy link
Contributor

cvializ commented Jul 1, 2019

I think the failing test might be a sign of broken behavior and not a flake. I'll take a look

@cvializ
Copy link
Contributor

cvializ commented Jul 1, 2019

I think this is the breaking change: the RRULE methods return values as local time formatted as a UTC date : / This is a breaking change you'd expect them to bump the major version for, not the minor.
https://github.com/jakubroztocil/rrule#important-use-utc-dates
I have a fix, I'll verify that it's working for the e2e tests too and push it here

@rsimha
Copy link
Contributor Author

rsimha commented Jul 1, 2019

This is a breaking change you'd expect them to bump the major version for, not the minor.

Nice catch. I've removed the commit that skips a test from this PR, so all that's left to do now is fix the existing tests, and manually test the component to make sure it works, and all the code is included.

@rsimha
Copy link
Contributor Author

rsimha commented Jul 1, 2019

@cvializ I tested this with your commit included, and verified that:

  • All tests pass
  • Date picker scenarios in example and e2e test pages work
  • The rrule code is correctly included in dist/v0/amp-date-picker-0.1.js

This is now ready for review and merge once Travis goes green.

Copy link
Contributor

@cvializ cvializ left a comment

Choose a reason for hiding this comment

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

LGTM thanks for your help!

@rsimha rsimha merged commit 7dcb68a into ampproject:master Jul 1, 2019
@rsimha rsimha deleted the 2019-06-28-RRule branch July 1, 2019 17:20
thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants