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

Breaking changes in 1.10.4 #1360

Open
pycka opened this issue Jan 27, 2021 · 11 comments
Open

Breaking changes in 1.10.4 #1360

pycka opened this issue Jan 27, 2021 · 11 comments

Comments

@pycka
Copy link

pycka commented Jan 27, 2021

Describe the bug
#1338 introduced breaking change by removing DurationInputType and DurationAddType types from plugin namespace.
(4aca4b1#diff-ca2786d2c104006cc51bdc06948ed0fad7843dba6f8ca1570eaedea389c9da88L8)

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

Information

  • Day.js Version [e.g. v1.10.4]
  • OS: [e.g. iOS]
  • Browser [e.g. chrome 62]
  • Time zone: [e.g. GMT-07:00 DST (Pacific Daylight Time)]
@iamkun
Copy link
Owner

iamkun commented Jan 27, 2021

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
Copy link
Contributor

zardoy commented Jan 27, 2021

@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
Copy link
Contributor

zardoy commented Jan 27, 2021

Feel free to ask any questions!

@pycka
Copy link
Author

pycka commented Jan 27, 2021

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
Copy link
Owner

iamkun commented Jan 27, 2021

maybe because our user could use

Duration. DurationInputType

?

@zardoy
Copy link
Contributor

zardoy commented Jan 27, 2021

@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
Copy link
Contributor

zardoy commented Jan 27, 2021

@iamkun Also could you please merge master branch?

@iamkun
Copy link
Owner

iamkun commented Jan 28, 2021

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

@zardoy
Copy link
Contributor

zardoy commented Jan 28, 2021

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
Copy link
Author

pycka commented Jan 29, 2021

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

@hisuwh
Copy link

hisuwh commented Feb 3, 2021

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

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

No branches or pull requests

4 participants