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

🏗 Switch from third_party/rrule to the rrule module #22946

Merged
merged 3 commits into from Jun 24, 2019
Merged

🏗 Switch from third_party/rrule to the rrule module #22946

merged 3 commits into from Jun 24, 2019

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Jun 20, 2019

This PR switches extensions/amp-date-picker/0.1/dates-list.js from third_party/rrule to the rrule module. The aim is to address the TODO in 09eddaa

Follow up to #22914

@rsimha rsimha self-assigned this Jun 20, 2019
yarn.lock Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@rsimha
Copy link
Contributor Author

rsimha commented Jun 20, 2019

I see one new type error: https://travis-ci.org/ampproject/amphtml/jobs/548421199#L1035 I fear this is due to our custom version of closure compiler, and how it does pure imports. If so, this PR might have to wait until google/closure-compiler#3041 is fixed. Edit: Got things to work by suppressing the closure error for pure imports.

@jridgewell Meanwhile, I think you can re-enable the rule for ChildNode.p.after() and whitelist extensions/amp-date-picker/0.1/dates-list.js, similar to ChildNode.p.before() directly above it

requirement: {
type: BANNED_PROPERTY_CALL
error_message: 'ChildNode.p.before() is unusual. Please use Node.p.insertBefore()'
value: 'Element.prototype.before'
value: 'DocumentType.prototype.before'
value: 'CharacterData.prototype.before'
# It's really hard to properly type check this one.
whitelist: 'extensions/amp-date-picker/0.1/dates-list.js'
}

# TODO(rsimha): Reenable when we move RRule to a node module.
# requirement: {
# type: BANNED_PROPERTY_CALL
# error_message: 'ChildNode.p.after() is unusual. Please use Node.p.insertBefore()'
# value: 'Element.prototype.after'
# value: 'DocumentType.prototype.after'
# value: 'CharacterData.prototype.after'
# }

@rsimha
Copy link
Contributor Author

rsimha commented Jun 24, 2019

Got type checks and tests to work by rolling back to rrule v2.2.0, the version originally checked in to third_party/, which also removes the unnecessary dependency on luxon.

This PR is ready for another review.

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

@rsimha rsimha merged commit 595737b into ampproject:master Jun 24, 2019
@rsimha rsimha deleted the 2019-06-20-RRule branch June 24, 2019 19:57
@jridgewell
Copy link
Contributor

@rsimha Is it possible to drop the conformance config exceptions with this change?

@rsimha
Copy link
Contributor Author

rsimha commented Jun 24, 2019

@rsimha Is it possible to drop the conformance config exceptions with this change?

I just tried, and am seeing these two conformance violations:

extensions/amp-date-picker/0.1/dates-list.js:83: ERROR - Possible violation: ChildNode.p.after() is unusual. Please use Node.p.insertBefore()
The type information available for this expression is too loose to ensure conformance.
      .map(rrule => rrule.after(date))
                    ^^^^^^^^^^^

extensions/amp-date-picker/0.1/dates-list.js:113: ERROR - Possible violation: ChildNode.p.before() is unusual. Please use Node.p.insertBefore()
The type information available for this expression is too loose to ensure conformance.
      const rruleDay = this.moment_(rrule.before(nextDate));
                                    ^^^^^^^^^^^^

I think this can be fixed by providing type info for this.rrulestrs_. @cvializ, do you know what the type is supposed to be?

@jridgewell
Copy link
Contributor

They should be RRule types.

@rsimha
Copy link
Contributor Author

rsimha commented Jun 24, 2019

In that case, we're likely going to have to wait until google/closure-compiler#3041 is fixed, which will allow us to use the {Array<RRule>} type annotation for this.rrulestrs_. Until then, we'll see this error.

extensions/amp-date-picker/0.1/dates-list.js:43: ERROR - Bad type annotation. Unknown type RRule
    /** @private @const {Array<RRule>} */
                               ^

Importing RRule from the rrule module yields this slightly different error:

extensions/amp-date-picker/0.1/dates-list.js:43: ERROR - Bad type annotation. Unknown type RRule$$module$rrule
    /** @private @const {Array<RRule>} */
                               ^

@rsimha
Copy link
Contributor Author

rsimha commented Jun 24, 2019

I tried a different thing: casting the rrule param to RRule, and got these errors:

extensions/amp-date-picker/0.1/dates-list.js:83: ERROR - Property after never defined on rrule
      .map((/** {RRule} */ rrule) => rrule.after(date))
                                           ^^^^^

extensions/amp-date-picker/0.1/dates-list.js:113: ERROR - Property before never defined on rrule
      const rruleDay = this.moment_(rrule.before(nextDate));
                                          ^^^^^^

@rsimha
Copy link
Contributor Author

rsimha commented Jun 25, 2019

The missing ingredient was an entry in the externs file. Done in #23001.

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