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

The On bit for JSObject displays as JSObjectRepType which is weird #3684

Open
kevmoo opened this issue Feb 27, 2024 · 11 comments · Fixed by #3685
Open

The On bit for JSObject displays as JSObjectRepType which is weird #3684

kevmoo opened this issue Feb 27, 2024 · 11 comments · Fixed by #3685
Assignees
Labels
P3 A lower priority bug or feature request type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@kevmoo
Copy link
Member

kevmoo commented Feb 27, 2024

See https://pub.dev/documentation/web/0.5.0/web/HTMLElement-extension-type.html

image

Also!! the on bit for HTMLElement is meant to be ~hidden.

extension type HTMLElement._(JSObject _) implements Element, JSObject {
  external factory HTMLElement();

Either way, JSObjectRepType is certainly not what we want!

@kevmoo kevmoo added the type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) label Feb 27, 2024
@srawlins srawlins self-assigned this Feb 27, 2024
@kevmoo
Copy link
Member Author

kevmoo commented Feb 28, 2024

@kevmoo kevmoo reopened this Feb 28, 2024
@kevmoo
Copy link
Member Author

kevmoo commented Feb 28, 2024

To be clear, this MAY just be due to the SDK type. Things are fixed with pkg:web

@srawlins
Copy link
Member

I don't expect this to be fixed in the SDK. It is only fixed in this repo.

40c470e does not include d8e7f99.

https://github.com/dart-lang/dartdoc/commits/main/

@kevmoo
Copy link
Member Author

kevmoo commented Mar 3, 2024

Looking at https://api.dart.dev/main/42ccd79cd12a640359fb729537f5b14a06055188/dart-js_interop/JSArray-extension-type.html

this includes dartdoc at eed92d3

am I reading this wrong?

@srawlins
Copy link
Member

srawlins commented Mar 3, 2024

Hey Kevin, sorry this isn't fixed.

What do you expect this to say?

Given this code:

// interceptors.dart
class JSObject { ... }

// js_types.dart
typedef JSObjectRepType = interceptors.JSObject;

// js_interop.dart
extension type JSObject._(JSObjectRepType _jsObject) implements JSAny { ... }

The representation type of JSObject (the extension type) is JSObjectRepType. AFAIK we do not attempt to unravel typedefs in dartdoc. If the code says JSObjectRepType is the representation type, then we use that.

We do have some mechanisms for working around "package internal" types. Like if class A extends class _B which extends class C, then we can usually display the more useful info that "class A extends class C".

I think there are a few principles we should not depart from though:

  • Representation types should be documented; it's impossible to create an instance of an extension type unless you know the representation type.
  • Typedefs should generally not be unraveled. The goal, the singular purpose, of a typedef, is to textually replace one type with another.

@kevmoo
Copy link
Member Author

kevmoo commented Mar 4, 2024

We should maybe chat with @srujzs and/or @sigmundch about how to deal with these types. If we should special-case or what

@srujzs
Copy link
Contributor

srujzs commented Mar 4, 2024

There are two separate cases here (and one of them may be addressed already - I can't tell since it's not on the website yet :)):

  1. How we depict the representation type in dart:js_interop in the documentation. If we have to depict a representation type here, it should certainly be JSObjectRepType instead of the underlying type for our purposes. The value of that typedef is platform-dependent, so we shouldn't unravel it. It may be confusing, however, for users to see JSObjectRepType.

One proposal to address that was that since the representation field is "private" (starts with an underscore), that may be a signal to the docs to not show the representation type. That determination may make sense here but not elsewhere. Another option is just not showing the representation type for these specific SDK types as they're likely to confuse users.

it's impossible to create an instance of an extension type unless you know the representation type.

For these types, you're always using some other mechanism to get an instance, and users can never use the representation type as they're declared in a private library and never exported. Of course, that distinction is very specific to these types and may not make sense for any other extension type.

Realistically, I don't feel strongly enough about not showing the representation type for these types and care more about 2.

  1. package:web types should show the representation type and not the extension type erasure i.e. JSObject and not JSObjectRepType. This is very useful as it shows how interop extension types compose on top of JSObject rather than some typedef users can never access. I also think this is generally a good idea everywhere as this is part of the extension type's signature. It looks like @srawlins already has a PR for this?

@sigmundch
Copy link
Member

An interesting piece about (2), is that it may be more intuitive in general for all extension types, outside the js-interop realm. For example, if you have:

extension type F(int x) {}
extension type G(F f) {}

I'd prefer G to be documented as having an F representation type. That preserves the intent that extension types provide an abstraction that can be used statically everywhere. When it comes to G's constructor, I really must have F in the documentation, otherwise users will think they can create instances like G(3) instead of G(F(3))

@srawlins
Copy link
Member

srawlins commented Mar 5, 2024

Agree with @srujzs 's and @sigmundch 's comments above 😁

One proposal to address that was that since the representation field is "private" (starts with an underscore), that may be a signal to the docs to not show the representation type.

I don't think this fits. If I make the representation field private, that just means that external libraries cannot access it, and is orthogonal to whether the representation type should be documented. In fact, I have to imagine that the guidance for a representation field is the same as any other instance field: Make it private unless it is necessary (or some level of useful) outside the library. So most (the vast majority of?) representation fields should be private. Regardless of whether it is intended that they are public constructable.

If we want special signals that say "not constructable! don't document the rep type!", we can make that. Since this is all in the SDK, it cannot be a package:meta annotation. It wouldn't be allowed as a dart:core annotation. So we can make it a dart doc comment directive, something like /// @nodoc-representation-type.

@srujzs
Copy link
Contributor

srujzs commented Mar 5, 2024

I don't think this fits. If I make the representation field private, that just means that external libraries cannot access it, and is orthogonal to whether the representation type should be documented.

Agreed, it seems like it might be a bad fit. The representation type is always visible in the type signature, and that should be a separate determination from the field name.

An annotation might be nice, but it might make sense to wait here before implementing anything like that. While showing the representation types for dart:js_interop types may be confusing, the fact that you can't actually access and inspect those representation types through the docs since they're in a private SDK library leads me to believe the confusion will be limited,

@srawlins
Copy link
Member

srawlins commented Mar 5, 2024

Sounds good!

@bwilkerson bwilkerson added the P3 A lower priority bug or feature request label Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 A lower priority bug or feature request type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants