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

Remove d.ts files #21174

Merged
merged 1 commit into from
Jan 30, 2021
Merged

Remove d.ts files #21174

merged 1 commit into from
Jan 30, 2021

Conversation

mrdoob
Copy link
Owner

@mrdoob mrdoob commented Jan 29, 2021

Related issue: #15597

Description

This PR removes all .d.ts files from the repo.

TypeScript definitions were originally maintained in https://github.com/DefinitelyTyped/DefinitelyTyped but 2 years ago we decided to include them in the repo hoping that it would ensue they would be in sync.

The side effect of that decision was a considerable increase on maintenance burden and made things harder for new contributors too. For the sake of continuity of the project I think it's best to move them back to DefinitelyTyped (or any other repo interested in maintaining them).

During the 2 years that these files were in the repo I think we did a good job at bringing them in sync but it's time to delegate the maintenance.

@squidfunk
Copy link

Hmm, I guess this will break a lot of projects. I agree with removing the generics, but shouldn't we at least wait, until there's an alternative before removing all types? This renders Three.js pretty much unusable in my TS-only codebase.

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 29, 2021

This would only remove them from dev. We have until February 24 (0.126.0 on npm) to find an alternative.

@DavidPeicho
Copy link
Contributor

It's really too bad because it brings a lot of value for TS users. I understand it's a lot of effort so maybe we could choose someone that can help maintain the typings?

@squidfunk
Copy link

squidfunk commented Jan 29, 2021

Okay, cool. I understand that you don't want to maintain them, but I'm strongly in favor of a solid migration plan. DefinitelyTyped sounds like a good alternative if you don't want to have them in this repository. If it's as easy as dropping in @types/three, that would be ideal. Note that there might be some problems with the examples folder though.

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 29, 2021

@squidfunk Would you like to take on this task?

@squidfunk
Copy link

@mrdoob Thanks for asking. I'm afraid, I can't do that as I already have too many open source projects to maintain. I'm sorry.

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 29, 2021

@squidfunk By "task" I didn't mean maintaining them, I meant dropping them in @types/three.

@xawill
Copy link
Contributor

xawill commented Jan 29, 2021

Sorry to all TS users that my PR triggered this...

@joshuaellis
Copy link
Contributor

Wouldn't this be a good opportunity to do an RFC from the community?

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 29, 2021

Already did: https://twitter.com/threejs_org/status/1355153151310098433

@DavidPeicho
Copy link
Contributor

What do you think about keeping them in this repo and someone is assigned as the main person to handle types? Not sure how that could go.

@joshuaellis
Copy link
Contributor

@mrdoob I don't mind putting them on DefinitelyTyped if that's the decision we want to do.

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 29, 2021

@DavidPeicho TS related conversations have been too noisy and distracting. I would prefer if all that activity happened somewhere else.

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 29, 2021

@joshuaellis That'd be great. Let me know when it's done 🙏

@CodyJasonBennett
Copy link
Contributor

CodyJasonBennett commented Jan 29, 2021

Is there any way we can make this work with JSDoc, or does that have the same pitfalls?

I worry about the types further lagging behind contributions made on this repository.

@Methuselah96
Copy link
Contributor

I would be willing to help maintain the types on DefinitelyTyped.

@DavidPeicho
Copy link
Contributor

Okay I guess then we can move them, and start from there. The library is published every month we basically need to have the types ready at each release :)

@Methuselah96
Copy link
Contributor

Methuselah96 commented Jan 29, 2021

(P.S. The types were significantly better after they moved to this repo since it was very easy for the types to get out-of-date on DefinitelyTyped. It was also a huge advantage to have the code and types in-sync to help with breaking changes. However I agree with @mrdoob's reasoning and I don't think it's a fair expectation for any project to have to maintain types if it's causing unhelpful noise. Thank you for trying it out and the period in which the types were always in-sync.)

@Methuselah96
Copy link
Contributor

Methuselah96 commented Jan 29, 2021

@DavidPeicho Yeah, it won't be easy and we should expect some lag between the package publishing and updated types publishing (hopefully not more than a week of lag).

@Methuselah96
Copy link
Contributor

Methuselah96 commented Jan 29, 2021

@mrdoob Note that I am also willing to take ownership of the type definitions on this repo (in response to this comment and this comment) which would mean that we can continue to have high-quality types that are always up-to-date with each release. You could just mention me on any TypeScript related issues and PRs and I could handle it/give my recommendation.

I think this would be the better situation for the developers' overall-all experience and it would be make significantly less work for people maintaining the types (so they don't have to cross-reference all the changes and try to get releases out ASAP), but I do understand that it does still impact you. However I think that I could take a lot of the work that you had to do in #19702 and #21172 and handle it as much as I can before letting you make an executive decision (if necessary/desired).

@joshuaellis
Copy link
Contributor

@mrdoob Note that I am also willing to take ownership of the type definitions on this repo (in response to this comment and this comment) which would mean that we can continue to have high-quality types that are always up-to-date with each release. You could just mention me on any TypeScript related issues and PRs and I could handle it/give my recommendation.

I think this would be the better situation for the developer's overall-all experience and it would be make significantly less work for people maintaining the types (so they don't have to cross-reference all the changes and try to get releases out ASAP), but I do understand that it does still impact you. However I think that I could take a lot of the work that you had to do in #19702 and #21172 and handle it as much as I can before letting you make an executive decision (if necessary/desired).

I second this, I am also happy to help on the TS front in the same way as @Methuselah96.

@CodyJasonBennett
Copy link
Contributor

CodyJasonBennett commented Jan 29, 2021

I'm concerned about the stability of types and related documentation for modules in the examples folder, as this is often their only form of documentation.

If the issue is with the burden of maintaining these modules and their types/docs in addition to the three core, perhaps this should be a larger conversation?

I worry that this is only a symptom of a larger problem.

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 29, 2021

@Methuselah96 Many thanks for stepping up! 🙏

I think I'd prefer if the types are on DefinitelyTyped for now though. By the way, one benefit of having it outside is that you can update the types any time instead of just once a month.

As per the next steps... We just need to copy the current types to the DefinitelyTyped repo and change this line to @types/three, right?

@Methuselah96
Copy link
Contributor

@mrdoob Yeah, we just need to copy the types and users will have to install @types/three separately from three. The line you referenced would be removed altogether (which is already done as part of this PR).

Sounds like @joshuaellis volunteered to do the work to put the types on DefinitelyTyped, so I'll wait until that's done unless he would prefer I do it.

@joshuaellis
Copy link
Contributor

@Methuselah96, @mrdoob I am indeed already on it. @Methuselah96 i'll add you as an owner to the code while i'm at it.

@gkjohnson
Copy link
Collaborator

I'm not an expert in TS but is it possible to maintain type definitions in a different repo owned by the three.js community? So users would have a more dedicated location to direct issues, questions, discussions, and PRs and the types will be easier to browse? It could go in the the threejs Github org where people within the community could maintain it but would direct noise away from the core project.

@donmccurdy
Copy link
Collaborator

I'm not an expert in TS but is it possible to maintain type definitions in a different repo owned by the three.js community?

p5.js did this (https://github.com/p5-types/p5.ts), it's definitely an option. I don't have a preference and would be happy to defer to any TypeScript users here, especially those able to help maintain the type definitions. Pros: Might be easier to include tooling that tests the type definitions. Cons: Can't use the standard @types/* namespace if it's not in DefinitelyTyped, I expect.

@joshuaellis
Copy link
Contributor

I'm not an expert in TS but is it possible to maintain type definitions in a different repo owned by the three.js community?

IMO DefinitelyTyped is the defacto for this kind of thing, I think keeping it with other TS declarations is stronger in the long run. There's a bunch of repo bot help in DT too... I'm not sure the benefit of having it in it's own repo?

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jan 29, 2021

I'm not sure the benefit of having it in it's own repo?

There are some scripts in this repo now that check the d.ts files against official examples. I'm not sure how helpful those tests are, but having them around is the only benefit I can think of. I suppose the tests could be moved somewhere else and just run against DefinitelyTyped, too. Maintenance of the types has been better since they moved into this repo, but I don't know whether that was caused by (a) consuming maintainers' time, (b) higher visibility to the three.js community, or (c) something else.

@gkjohnson
Copy link
Collaborator

gkjohnson commented Jan 29, 2021

p5.js did this (https://github.com/p5-types/p5.ts), it's definitely an option.

Interesting. It looks like they basically generate the types in that repo and still ultimately upload them to DefinitelyTyped.

I'm not sure the benefit of having it in it's own repo?

As a user who doesn't use typescript but benefits from the typings in the form of intellisense I'd find it easier / less intimidating to more casually contribute fixes, issues, and updates to a project where I know my contributions will be vetted by people who know more than I do in the context of three.js. As evidenced by the many issues in this repo involving generics or different ways of representing typed information for three.js it's not always straight forward what the "right" answer should be.

It seems the types would ultimately have to wind up in DefinitelyTyped which could be automated with some kind of CI action but in my opinion the benefit would be lowering the barrier of entry and intimidation of contributing and ability to provide testing or linting infrastructure like @donmccurdy suggested.

It just occurred to me that moving everything to DefinitelyTyped entirely would probably lower the visibility of the types and therefore the smaller contributions to them, as well.

@fazouane-marouane
Copy link

Would a (possibly gradual) migration of three.js to typescript be a possibility ? I can spend some time on this if you're open to the idea.

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 29, 2021

@fazouane-marouane No, sorry. Better to use @bhouston's https://threeify.org/ or https://www.babylonjs.com/.

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 30, 2021

Okay, merging this for now.

@joshuaellis
Copy link
Contributor

I've started the move over, but it's not just a drag and drop situtation but it'll be done before r126. You can track the progress, and contribute here: DefinitelyTyped/DefinitelyTyped#50930

@Methuselah96
Copy link
Contributor

Methuselah96 commented Jan 30, 2021

I'm not sure the benefit of having it in it's own repo?

There are some scripts in this repo now that check the d.ts files against official examples. I'm not sure how helpful those tests are, but having them around is the only benefit I can think of. I suppose the tests could be moved somewhere else and just run against DefinitelyTyped, too. Maintenance of the types has been better since they moved into this repo, but I don't know whether that was caused by (a) consuming maintainers' time, (b) higher visibility to the three.js community, or (c) something else.

@donmccurdy We're planning on bringing the declaration files from the examples directory over as well. Is that what you are referring to, or are there other examples?

@donmccurdy
Copy link
Collaborator

Those declaration files are important, and I agree they should be kept. But what I meant was that repository contained a macro...

"lint-examples": "eslint examples/js examples/jsm --ext js --ext ts --ignore-pattern libs && tsc -p utils/build/tsconfig-examples.lint.json",

...which I believe did some general regression tests of the type definitions. It might be worth keeping something like that in DefinitelyTyped or elsewhere.

@Methuselah96
Copy link
Contributor

Got it. From what I can tell that command is just linting and then compiling the declaration files to make sure there are no issues. DefinitelyTyped already does that as part of its process so I don't think we'll see any regression in that regard.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 5, 2021

@mrdoob Since the d.ts files are now available at Definitely Typed and the respective npm package is ready how about announcing this PR in the forum? So we have something to refer when users are asking about this change. If okay, I will write some lines similar to the topic about Geometry.

https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/three
https://www.npmjs.com/package/@types/three

When I understand it correctly, TS users have to install the above npm package to get their typings back with r126, right?

@CodyJasonBennett
Copy link
Contributor

Yes, and it'll prompt them to look for types from @types/three.

@mrdoob
Copy link
Owner Author

mrdoob commented Feb 5, 2021

@Mugen87 Sounds good to me 👍

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 5, 2021

https://discourse.threejs.org/t/with-r126-typescript-type-declaration-files-are-located-at-definitelytyped/23157

@marquizzo
Copy link
Contributor

My only question is are types versioned in DefinitelyTyped? For example, if the latest Three.js version is r140 but I'm installing an old project that runs on revision r126, will I be able to install @types/[email protected]? Or does it always use the latest definition files? I'm looking at this discussion and it sounds like the .d.ts files aren't versioned.

@Methuselah96
Copy link
Contributor

Methuselah96 commented Feb 6, 2021

Types are indeed versioned on DefinitelyTyped and you will be able to install @types/[email protected] to get the correct types for r126.

@Methuselah96
Copy link
Contributor

Methuselah96 commented Feb 10, 2021

@gkjohnson @donmccurdy We decided to go ahead and host the types at https://github.com/three-types/three-ts-types and then sync them to DefinitelyTyped (and publish them as @types/three). I believe that should gives us the benefits of using our own repo along with the benefits of hosting on DefinitelyTyped.

@beginor
Copy link
Contributor

beginor commented Feb 25, 2021

I think its too bad to remove the *.d.ts from this repo, those files should be generated from source file, not be maintained manually.

Typescript does not only work with ts files, but alwo can works with js files, please refer to this post : TypeScript without TypeScript -- JSDoc superpowers

If we add JSDoc to the source cold, we can generate excellent d.ts files with tsc.

@beginor
Copy link
Contributor

beginor commented Feb 25, 2021

There is a great video on that. But there’s a lot, lot more to it than a couple of basic types in comments. Turns out working with JSDoc type gets you very far.

https://www.youtube.com/watch?v=YHvqbeh_n9U

Repository owner locked as resolved and limited conversation to collaborators Feb 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.