iamkun / dayjs

⏰ Day.js 2kB immutable date-time library alternative to Moment.js with the same modern API
https://day.js.org
MIT License
46.98k stars 2.3k forks source link

Breaking changes in 1.10.4 #1360

Open pycka opened 3 years ago

pycka commented 3 years ago

Describe the bug

1338 introduced breaking change by removing DurationInputType and DurationAddType types from plugin namespace.

(https://github.com/iamkun/dayjs/commit/4aca4b1b584a15de1146d929f95c944594032f20#diff-ca2786d2c104006cc51bdc06948ed0fad7843dba6f8ca1570eaedea389c9da88L8)

Expected behavior No breaking changes within patch release (if following semantic versioning). Any plans to revert this change?

Information

iamkun commented 3 years ago

cc @zardoy can we still keep the same name?

It will be a fix in the next patch version to make sure there's no BC in a patch release.

zardoy commented 3 years ago

@pycka Why should we revert this change? We didn't update the major version because we didn't change any API, we just made types more strict. For example, maintainers of popular library, called Prisma never update the major version, when they change the type names, that aren't described in docs: https://github.com/prisma/prisma/releases/tag/2.15.0#Breaking%20Changes.

UPD: Prisma doesn't use semver. My bad.

@pycka Can I know where you use DuraionInputType?? It was not strict and could accept any kind of object: dayjs.duration({ somethingBad: "49" }); — not ok. I fully agree with you, that we should state this changes in a more clear way (as Prisma do). Also I would recommend to obtain the type of first argument in this way: Parameters<typeof dayjs.duration>[0] it will never break until BC in API is present.

UPD: Due strange TypeScript behavior we can't obtain the type of first argument using Parameters

zardoy commented 3 years ago

Feel free to ask any questions!

pycka commented 3 years ago

Thanks @iamkun and @zardoy for responding! Pulling this update broke our CI builds which in itself should be enough to call it a breaking change :) These types were publicly available before so we just used them for convenience when working with Durations. There was nothing in the source code that would imply that these are implementation details, especially considering another public interface Duration used to consume them (expressis verbis). I don't know Prisma, nor I spend too much with the docs, so it's hard for me to relate to that. From TypeScript point of view these changes weren't backward compatible.

iamkun commented 3 years ago

maybe because our user could use

Duration. DurationInputType

?

zardoy commented 3 years ago

@pycka I have contacted Prisma team and I realised that they don't use SemVer.

From TypeScript point of view these changes weren't backward compatible.

Yeah, you're right. I just removed DurationInputType and DurationAddType type definitions. I have already opened PR that restores them :( But unfortunately I can't even deprecate them, because there is no alternative to them (see updated comment above).

@iamkun, please keep this issue open, until I realise how we could replace these old type definitions. After that we could deprecate them and remove in new breaking release.

zardoy commented 3 years ago

@iamkun Also could you please merge master branch?

iamkun commented 3 years ago

Seems the added type DurationInputType is not used. I'm not good at TS, can we do something to avoid this?

zardoy commented 3 years ago

Seems the added type DurationInputType is not used. I'm not good at TS, can we do something to avoid this?

It is not used in our implementation. But in previous version of dayjs @pycka could use them, because they were publicy available: image

And of course new version of dayjs doesn't contain these type definitions.

These types were publicly available before so we just used them for convenience when working with Durations.

I'm still wondering how is this even possible? We didn't use export keyword to export them from the namespace. I think I need more time for investigation here.

P.S. Sorry for screenshots here, TypeScript playground doesn't work with nested paths for some reason.

pycka commented 3 years ago

import { DurationInputType } from 'dayjs/plugin/duration';

hisuwh commented 3 years ago

I'm still wondering how is this even possible? We didn't use export keyword to export them from the namespace

I think this is the important part here. You are exporting the namespace which presumably exports all the members of it

image