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

Should index.d.ts use readonly field: number or new syntax get field(): number ? #678

Closed
justingrant opened this issue Jun 15, 2020 · 6 comments
Labels
typescript Related to the TypeScript bindings

Comments

@justingrant
Copy link
Collaborator

Over the weekend I learned about a relatively-recent change in how TypeScript models property getters in .d.ts files. The old (pre-TS 3.7) approach was readonly field: number which is indistinguishable from a regular field except it's read-only because there's only a getter. The TS 3.7+ way is to expose the fact that there's a getter function: get field(): number.

microsoft/TypeScript#27644 has more context to explain why this change was made on the TS side. @rbuckton and @littledan and @ljharb were all on that thread so may have more to add.

Apparently, per microsoft/TypeScript#33939 this is a breaking change for TS users running 3.6 or lower who try to import types declared with the new syntax. So I'm not sure which format should be in Temporal types, and if "new format" then I'm not sure when would be best to make the switch. @rbuckton do you have an opinion?

@justingrant justingrant changed the title Should index.d.ts readonly field: number or new syntax get field(): number ? Should index.d.ts use readonly field: number or new syntax get field(): number ? Jun 15, 2020
@rbuckton
Copy link

rbuckton commented Jun 15, 2020

Apparently, per microsoft/TypeScript#33939 this is a breaking change for TS users running 3.6 or lower who try to import types declared with the new syntax.

Are you familiar with "typesVersions"? Its a package.json entry TypeScript recognizes to pick a different index.d.ts file bases on the version of TypeScript that is consuming the package. That would allow you to specify get field(): number for newer versions of TS. It does add maintenance complexity, as it requires maintaining two versions of the .d.ts file.

@justingrant
Copy link
Collaborator Author

Yep, I was hoping to avoid typesVersions. Maybe the right approach would be to use a simple transform to start with one variation (e.g. get()) and transform to the other variation. Then we could use typesVersion without the work of maintaining two different files.

@ptomato ptomato added this to the Stage 3 milestone Sep 17, 2020
@ptomato
Copy link
Collaborator

ptomato commented Jan 13, 2021

Any new insights on this now that TypeScript 3.7 has been out for a while?

Regardless, I don't think it's a prerequisite for Stage 3, so I'm removing it from that milestone.

@ptomato ptomato removed this from the Stage 3 milestone Jan 13, 2021
@ptomato ptomato added the typescript Related to the TypeScript bindings label Jan 13, 2021
@justingrant
Copy link
Collaborator Author

I'm inclined to leave the old syntax (readonly). AFAIK there's not much harm in leaving it that way. If the TS folks want to change it later when they build the types into TS's built-in JS typings that seems fine too. Punt for now.

@ljharb
Copy link
Member

ljharb commented Jan 13, 2021

Note that once stage 3 is achieved, the previously stated intention is to deprecate the polyfill anyways (if someone wants to make a new one in a non-tc39 org with the same code, that's up to them).

@ptomato
Copy link
Collaborator

ptomato commented Jan 14, 2021

OK, I'll close this for now.

@ptomato ptomato closed this as completed Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typescript Related to the TypeScript bindings
Projects
None yet
Development

No branches or pull requests

4 participants