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

[v2.0.0] Remove Carbon dependency #86

Closed
AlexOlival opened this issue Oct 8, 2020 · 5 comments
Closed

[v2.0.0] Remove Carbon dependency #86

AlexOlival opened this issue Oct 8, 2020 · 5 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@AlexOlival
Copy link
Collaborator

AlexOlival commented Oct 8, 2020

As I'm working on the JS/TS/npm port, I'm striving to keep dependencies to a minimum - something that the npm ecosystem is plagued with.

However we ourselves are forcing a dependency on a DateTime wrapper library that, no matter how great it is, not all projects will use.
PHP's DateTime class is fine and we should refactor our date handling to it, including Citizen's return type.

@AlexOlival AlexOlival added the enhancement New feature or request label Oct 8, 2020
@AlexOlival AlexOlival added the good first issue Good for newcomers label Oct 8, 2020
@AlexOlival AlexOlival removed this from the v1.3.0 - The New World Update milestone Oct 9, 2020
@AlexOlival AlexOlival changed the title [v1.3.0] Remove Carbon dependency [v2.0.0] Remove Carbon dependency Oct 9, 2020
@AlexOlival
Copy link
Collaborator Author

An addendum: this will be a breaking change, so we're talking a major version here.

@AlexOlival
Copy link
Collaborator Author

Alternatively, we could deprecate the method returning the Carbon instance, and then later remove it altogether...
Thoughts @JoaoFSCruz ?

@JoaoFSCruz
Copy link
Collaborator

Currently we are using Carbon when manipulating dates in the extractors and to define the date of birth in the Citizen model. If we just deprecate the Citizen's method to return a DateTime instance instead of a Carbon instance we are not making that of a difference.

Maybe really push for the bigger change and totally lose Carbon. 🤷‍♂️

@AlexOlival
Copy link
Collaborator Author

Maybe really push for the bigger change and totally lose Carbon. man_shrugging

It would however mean that we could have a smoother transition to getting rid of Carbon without introducing a breaking change out of the blue. Something like v1.3.0 having a new getDateOfBirthNative() method alongside the old Carbon returning one, and then on v2.0, the original getDateOfBirth() would be rewritten to return a DateTime.

@JoaoFSCruz
Copy link
Collaborator

I may have misunderstood 😅

When upgrading to a later version we can get rid of Carbon. But until then we can implement another method returning a DateTime instance for a smooth transition.

That sounds totally fine 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants