-
-
Notifications
You must be signed in to change notification settings - Fork 519
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
Remove luxon #508
Remove luxon #508
Conversation
Codecov Report
@@ Coverage Diff @@
## master #508 +/- ##
==========================================
- Coverage 88.50% 88.49% -0.02%
==========================================
Files 29 29
Lines 2097 2094 -3
Branches 584 584
==========================================
- Hits 1856 1853 -3
Misses 241 241
Continue to review full report at Codecov.
|
@@ -35,8 +35,6 @@ Alternatively, download manually: | |||
|
|||
* [rrule.min.js](https://jakubroztocil.github.io/rrule/dist/es5/rrule.min.js) (bundled, minified) | |||
* [rrule.js](https://jakubroztocil.github.io/rrule/dist/es5/rrule.js) (bundled, not minified) | |||
* [rrule-tz.min.js](https://jakubroztocil.github.io/rrule/dist/es5/rrule-tz.min.js) (with timezone support, bundled, minified) | |||
* [rrule-tz.js](https://jakubroztocil.github.io/rrule/dist/es5/rrule-tz.js) (with timezone support, bundled, not minified) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are projects loading from these github.io urls, for example using a <script> tag in an html file, this change will break those projects. I recommend we make a copy of rrule.min.js and rrule.js to rrule-tz.min.js and rrule-tz.js and continue to place them under dist folder.
But on the other hand, I don't know can we push users of these 2 files to switch. If we do not want to keep provide these files forever, eventually we need to stop and whenever we stop, it will break some body, so why not break it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope nobody is linking directly to these files in a production app! It would be a very bad idea, since they're not versioned and can change unpredictably. Thus, I think it's a good idea to discourage that use case. These files are for direct download only. I would support removing even these links, and encourage everyone to just use npm as the source of truth for the package, but I think that's out of scope for this PR.
In README, there is still a reference to luxon and example |
I chose to leave the Luxon reference in the README, since it's mentioned only as an example of a library that can be used to manipulate dates & time zones. Other packages eg. date-fns & day.js could just as easily work in place of Luxon to achieve the same result. I don't think it's necessary for this package's README to give examples in every possible Date library, so I think it's acceptable to leave a single example in an arbitrary package, in this case Luxon. |
So does this mean I can do this: And the occurrences returned will be like this: |
Addresses #501
Note it is now possible to avoid using any 3rd party dependency, since browser support for the
Intl
API is now fairly widespread.