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

Add policy for undocumented public items #172

Closed
ycw opened this issue Feb 15, 2022 · 10 comments
Closed

Add policy for undocumented public items #172

ycw opened this issue Feb 15, 2022 · 10 comments
Labels
enhancement New feature or request

Comments

@ycw
Copy link
Contributor

ycw commented Feb 15, 2022

Describe the feature you'd like:

Since types have to model 1:1, this will expose all items including the undocumented ones which are not supposed to be 'public', i suggest adding @remarks to those items:

ex. Euler.d.ts

/**
 * @remarks
 * This item is not mentioned on the threejs docs.
 **/
static RotationOrders: string[];

ex. mrdoob/three.js#23466

/**
 * @remarks
 * This item is not mentioned on the threejs docs.
 **/
setTexture( .. );

In case the items are removed from the src:

- /**
-  * @remarks
-  * This item is not mentioned on the threejs docs.
-  **/
-  setTexture( .. );
@ycw ycw added the enhancement New feature or request label Feb 15, 2022
@joshuaellis
Copy link
Member

Why do you think they have to model 1:1? I've (and many others) have used this lib with three following the ethos of not including internal or private methods and haven't come up to any roadblocks before.

As a side note, if we were to agree with this proposal, it's something I would expect the community to contribute to as the sheer number of files is unmanageable.

@donmccurdy
Copy link
Contributor

I don't have a preference for whether internal properties and methods are included in the types or not, but if they are included, the @internal tag might be appropriate. See TypeDoc documentation on @internal.

@ycw
Copy link
Contributor Author

ycw commented Feb 15, 2022

Why do you think they have to model 1:1? I've (and many others) have used this lib with three following the ethos of not including internal or private methods and haven't come up to any roadblocks before.

@joshuaellis 1:1 for public items, including undocumented, and pseudo privates ._x. ( but not private class fields .#x ).

@donmccurdy what if we don't model them 1:1 ? ex.

// blanket issue
class CssComposer extends EffectComposer {
  constructor() {
    super()
    this._width = '1024px' // passes tsc but breaks effectcomposer's internal

As a side note, if we were to agree with this proposal, it's something I would expect the community to contribute to as the sheer number of files is unmanageable.

I think we should first ask 3js when it will adopt private class fields.

@donmccurdy
Copy link
Contributor

I would not recommend including private fields in the type declarations, regardless of whether they're pseudo-private or private class fields — the intention is the same.

@ycw
Copy link
Contributor Author

ycw commented Feb 15, 2022

@donmccurdy The intention is the same, but the impl is totally different, pseudo privates suffer from this whereas private class fields don't. Hope the updated demo helps 🤪

@ycw
Copy link
Contributor Author

ycw commented Feb 15, 2022

but if they are included, the @internal tag might be appropriate. See TypeDoc documentation on @internal.

@donmccurdy why typedoc (the tool)? do you mean tsdoc (the spec)? note that @remarks is in core, @internal is in discretionary

@donmccurdy
Copy link
Contributor

I do understand the technical differences between the two – still, my advice would be to omit private properties (of any type) and focus on the public API.

The @remarks tag has no semantic meaning — just human-readable notes — whereas @internal implies exactly the right meaning here: accessing these fields externally is error-prone and not recommended. To say that the fields are just "not mentioned" would be misleading — they're deliberately omitted because they are not meant for external use.

@ycw
Copy link
Contributor Author

ycw commented Feb 15, 2022

I do understand the technical differences between the two – still, my advice would be to omit private properties (of any type) and focus on the public API.

Great, so you understand that pseudo privates are techically public , right? the demo had examined that if their types are missing from three-ts-types, a derived class might blanket a pseudo private item in the base by accident, and screwup the base methods SLIENTLY. If missing types of public items are acceptable, doesn't it break the "Priorities and goals" of this project?

The @remarks tag has no semantic meaning — just human-readable notes — whereas @internal implies exactly the right meaning here: accessing these fields externally is error-prone and not recommended. To say that the fields are just "not mentioned" would be misleading — they're deliberately omitted because they are not meant for external use.

I agree that @internal is a more accurate mod for pseudo private items, but i'm not sure if it fits all undocumented public items, say WebGLRenderTarget.prototype.setTexture, is it internal, or just docs is out-sync?

@donmccurdy
Copy link
Contributor

The comments above are just my opinion. You and the other contributors may of course make your own decision on this. 🙂

@joshuaellis
Copy link
Member

I agree with @donmccurdy, we're only going to expose the public facing api types, i've updated the readme to reflect this.
This is typically inline with other libraries that have their types hosted through DefinitelyTyped e.g. react. You can of course extend these types to include the private methods and properties if you so wish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants