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

Luxonless binary should not contain any luxon imports. #410

Merged
merged 1 commit into from
Aug 20, 2020

Conversation

samouri
Copy link
Contributor

@samouri samouri commented Jun 23, 2020

summary
Fixes #344, and maybe #402.

Currently the build produced without timezone support still references luxon and expects the code to throw a TypeError if incorrectly used (done via webpack externals). You can verify this by searching for "luxon" within the contents of dist/es5/rrule.min.js.

This causes issue when mixed with compilers that don't have the same notion of an external, e.g. Closure Compiler (google/closure-compiler#954). Specifically, the AMP Project is running into when trying to build rrule into our amp-date-picker component (ampproject/amphtml#28887).

This PR uses NormalModuleReplacementPlugin to replace luxon with a fake implementation that immediately throws.

Uses NormalModuleReplacement to swap luxon with a fake.
@samouri
Copy link
Contributor Author

samouri commented Aug 20, 2020

@jakubroztocil / @davidgoli: friendly ping 🏓

@jkbrzt jkbrzt merged commit a96ba52 into jkbrzt:master Aug 20, 2020
@jkbrzt
Copy link
Owner

jkbrzt commented Aug 20, 2020

Thanks!

@samouri
Copy link
Contributor Author

samouri commented Aug 20, 2020

Thank you for merging this fix!
Not that there's any rush, but what's the release process like?

@jkbrzt
Copy link
Owner

jkbrzt commented Aug 21, 2020

@samouri I hope to find a time for it over the weekend (unless @davidgoli beats me to it).

Out of curiosity—what is your rrule.js use case?

@samouri
Copy link
Contributor Author

samouri commented Aug 21, 2020

@samouri I hope to find a time for it over the weekend (unless @davidgoli beats me to it).

Awesome!

Out of curiosity—what is your rrule.js use case?

I work for AMP, and we use rrule.js in the amp-date-picker component to support the highlighted/blocked attributes. We've managed to work around this issue in the meantime by manually patching the node_modules folder. See:

https://github.com/ampproject/amphtml/blob/a28fa129d225ce79d6b537f500c4540ae844caae/build-system/tasks/update-packages.js#L108-L119

@jkbrzt
Copy link
Owner

jkbrzt commented Aug 23, 2020

@samouri thanks for the background. v2.6.5 is out — https://www.npmjs.com/package/rrule The build process is broken, will need to fix that first.

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.

Crashes without luxon
2 participants