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

fix: Allow default initialization of DateTime/Duration/Interval #1643

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

Conversation

mbStavola
Copy link

@mbStavola mbStavola commented Jun 28, 2024

This PR default initializes the config parameter of the constructor for DateTime, Duration, and Interval in order to fix the following:

Cannot read properties of undefined (reading 'zone')

This can be encountered when using a third party serialization/deserialization library such as class-transformer as temporary objects may be constructed with new while waiting for any transformation logic to run. This is actually a very common issue for NestJS users.

Understandably, these constructors are meant to be private, but the unfortunate fact is that nothing can prevent new from being called on these types. Moreover, if you look at the constructor definitions it seems that the intent was to provide sane defaults already. With that in mind, it may make sense just to provide a default to sidestep the issue entirely and let the existing code handle things.

Ultimately, this is a minor adjustment which fixes some very real pain points.

Copy link

linux-foundation-easycla bot commented Jun 28, 2024

CLA Signed


The committers listed above are authorized under a signed CLA.

@mbStavola
Copy link
Author

I am also aware of this previous attempt, but I did not want to revive a year old dead PR. Additionally, that PR only covers DateTime.

@mbStavola
Copy link
Author

@icambron just saw the force-push notification here, was wondering if you had some time to review this?

@icambron
Copy link
Member

icambron commented Sep 4, 2024

Hmm, do the objects you construct this way actually work? I'm sort of guessing you'll get errors using them because e.g. values is not defined...

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.

2 participants