amp-date-picker: restore rrule support

[samouri]

summary
Reverts #25230, fixes #24682 and #25244. This PR replaces #28842.

We need rrule in order to support a couple of features within amp-date-picker. rrule imports luxon which has circular deps which breaks Google Closure Compiler.

Even though luxon is marked as an optional dependency of rrule within its package.json, it statically imports luxon making it essentially required. If Closure Compiler had the same notion of "externals" that webpack has we could potentially get around it by declaring luxon as external.

short term fix: patch datewithzone.js within node_modules.
long term fix: find a better solution: e.g. a timezone-less esm output target for rrule.

rrule in action

cc @caroqliu

[caroqliu]

One thing to consider here: I believe the original motivation for putting rrule directly in the node_modules is so it would get opted into auto version upgrades with renovate bot. If we want to use some modified unchanging version like rrule-without-luxon we should be able to place it in third_party/ instead of forking from your repo. /cc @rsimha

[samouri]

After testing, I've learned that the esm version of rrule adds 12.01KB whereas the es5 version adds 12.68KB.
I was surprised by how well the es5 version minifies based on usage (from 50kb!). It opens up another possibility for us to upstream a fix and use the built es5 version.

Testing this out now here jkbrzt/rrule#410

[samouri]

@rsimha: do you have any preference between using a fork of rrule that lives within third_party vs. patching it within the update_packages command?

[rsimha]

@rsimha: do you have any preference between using a fork of rrule that lives within third_party vs. patching it within the update_packages command?

Sorry about the late reply. My preference is to patch node_modules/rrule/ during gulp update-packages for two reasons: a) there is precedent for doing so, and b) we'll benefit from regular updates to rrule and still retain the ability to patch the JS file.

If you check in a copy to third_party, there's a risk that it will bit-rot over time, and if there ever are security vulnerabilities, we will never be alerted. (This is why we moved to node_modules in the first place.)

[samouri]

Oh no, while this works in uncompiled mode (browserify), when its compiled (cc) I get errors that I haven't been able to debug:

Also, @rsimha, re the second point:

b) we'll benefit from regular updates to rrule and still retain the benefit of being able to patch the JS file.

This is still suboptimal because an rrule update may break the patch. I don't see a good answer here without an upstreamed fix or manually periodically updating rrule.

[rsimha]

This is still suboptimal because an rrule update may break the patch. I don't see a good answer here without an upstreamed fix or manually periodically updating the src.

Yeah. Hopefully your patch is targeted enough that it will always work. Perhaps you could make the replacement optional and print a non-blocking warning if it's missing, so that if a future version stops importing luxon, we'll get to know without breaking compilation.

Edit: I should mention this is why we have Renovate. All subsequent rrule updates are tested, and if a breaking change is detected, we'll have a chance to fix it before updating the version used by AMP. (This is what we do today for closure compiler updates, many of which are potentially breaking.)

[samouri]

print a non-blocking warning if it's missing

I'm not sure this makes sense considering we run update-packages more often than we reinstall the local node_modules, which is why they use writeIfModified (on most runs, the node_modules have already been patched so no work is necessary).

---

Separately, something interesting I ran in to is that browserify and cc seem to deal with es6 imports of cjs modules differently. Namely that CC pretends everything is a default import, whereas browserify is flattened one level.

So this is valid in cc:

import * as rrule from 'rrule'
const rrulestr = rrule.default.rrulestr;

And this is a working representation in browserify:

import * as rrule from 'rrule'
const rrulestr = rrule.rrulestr;

[samouri]

@caroqliu / @rsimha: this is ready for a review now :)

[rsimha]

LGTM!

[rsimha]

OMG, you got this to work!

[caroqliu]

🔥 🔥 🔥

[jridgewell]

Nit: Please move this into tryParseRrulestr.

[samouri]

I think it makes more sense where it is, considering ideally it would even be import {rrulestr} from 'rrule';. We can discuss and i'd be happy to move it in a teeny future PR if it makes the most sense / I misunderstood the reasoning

[samouri]

sidebar: i realized that amp-date-picker currently accepts dates that are not the ISO 8601. We should either change the documentation or make the code more strict.