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

Replace moment with datedriver. #819

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

anisebamford
Copy link

  • Create the dateDriver service.
  • Add the vanilla dateDriver with the required moment api.
  • Replace all references to moment in the base code with dateDriver.
  • Update tests to expect dateDriver.
  • Add annotations to tests and test suites failing due to deprecations.

Issue Number

Link to issue this PR addresses. If no issue exists, please provide reasoning for PR.

Overview of PR

Provide an overview of the changes in this PR.

Don't forget to update the CHANGELOG.md file with any changes that are in this PR

* Create the dateDriver service.
* Add the vanilla dateDriver with the required moment api.
* Replace all references to moment in the base code with dateDriver.
* Update tests to expect dateDriver.
* Add annotations to tests and test suites failing due to deprecations.
@Ilaiwi
Copy link
Collaborator

Ilaiwi commented May 31, 2021

@bumpusfrancus really great work. Are parts of the implementation of vanillaDateDriver.js a derivative version of moment?
As for the next step for this, I will use this PR in the v1 version I am drafting. As for merging it with the current master, I will look into it but for it to be merged I would fix the skipped tests and add update the documentation

@deleonio
Copy link

deleonio commented Jun 8, 2021

@bumpusfrancus, @Ilaiwi

What does the removal of componentWillReceiveProps mean.

Are the skipped tests irrelevant in the future?

@anisebamford
Copy link
Author

anisebamford commented Jun 9, 2021

@deleonio Using componentWillReceiveProps causes React to log a warning, and due to the test runner's configuration, these tests fail. To get the tests that interact with the date drive to pass without reimplementing the hooks, I had to temporarily disable these tests. The tests are not irrelevant, but there is a different issue to replace component will receive props.

@anisebamford
Copy link
Author

anisebamford commented Jun 9, 2021

Yes, Vanilla date driver mimics moment for all the features used by the app, it does not provide any other part of Moment's API though. In the next version, I would recommend not exposing the date driver to custom components, and just use ISO8601 so that custom components can handle times however they like. That way, the date driver could be made non-configurable. The tests are already not passing. Requiring them to pass makes this issue blocked by #642. I'll look at writing some documentation.

While I followed Moment's API documentation while writing this, no part of the code was derived from Moment.

@Falkyouall
Copy link

is this getting merged sometime soon? I would love to get rid of momentjs

@fritjofwolf
Copy link

@Ilaiwi Can you tell me what the current status on this PR is? We are currently using your package and we are loving it. Unfortunately, our client will not allow us the use of moment.js in the future :(

@onewaypub
Copy link

This right here. We use the library too and it would be great if the problem would be fixed

@StoikovOleh
Copy link

Does it support timezones?

@anisebamford
Copy link
Author

anisebamford commented May 6, 2022

Does it support timezones?

All it does is use native js Date for formatting dates with moment date formats. It relies on the moment formats to attempt to maintain backwards compatibility. It can be replaced with a different library in the implementation.

The goal of this patch is to create the minimum possible implementation of a date library to keep the timeline working without external dependencies. It's not a fully fledged date library, and it shouldn't be one.

@TomHiller-swd
Copy link

@bumpusfrancus This works great but I noticed a bug where the mediumLong date format did not convert properly. It displayed as L, 01:00, L, 02:00, etc.

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.

8 participants