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

Deno.customInspect should be replaced with "denoCustomInspect" #9294

Closed
nayeemrmn opened this issue Jan 27, 2021 · 6 comments
Closed

Deno.customInspect should be replaced with "denoCustomInspect" #9294

nayeemrmn opened this issue Jan 27, 2021 · 6 comments
Labels
feat new feature (which has been agreed to/accepted) maybe 2.0 a potential feature for Deno 2.0 that needs further discussion runtime Relates to code in the runtime crate

Comments

@nayeemrmn
Copy link
Collaborator

  class Foo {
-   [Deno.customInspect]() {
+   denoCustomInspect() {
      return "foo";
    }
  }

This should be a progressively enhanced thing like WorkerOptions::deno. It should no-op in browsers, not require verbose feature detection to be compatible.

I think the alias should be made and Deno.customInspect should be deprecated for 1.8.0, and the latter should be removed for 2.0.

@kitsonk
Copy link
Contributor

kitsonk commented Jan 27, 2021

I'm strongly opposed to this. This is exactly what symbols were created for, so that objects don't need to be polluted with string property names that could collide, or be potentially observable in strange ways.

For internal runtime use, the "degrade to the web" is a non argument. For std library, there is a lot more to worry about "gracefully degrading" than a symbol. All Deno namespace APIs are a "problem" which is solvable by a Deno polyfill, including this symbol.

@nayeemrmn
Copy link
Collaborator Author

All Deno namespace APIs are a "problem" which is solvable by a Deno polyfill, including this symbol.

I disagree that Deno.customInspect and, say, Deno FS ops are similar in this respect. One declares an enhancing side-effect on logging that is either invoked (in Deno which can only write strings to terminals) or not invoked (in browser consoles which have rich logging by default) depending on the platform. The other calls a native binding the surrounding code will directly depend on, that either makes sense or doesn't make sense depending on the platform. The second should require a polyfill. I'm arguing that the first fundamentally shouldn't, and that currently the only thing about it that precludes compatibility is (literally) symbolic.

For internal runtime use, the "degrade to the web" is a non argument. For std library, there is a lot more to worry about "gracefully degrading" than a symbol.

I'm not sure what this means, I'm talking about users being able to write a user-space module that is both isomorphic and implements a custom inspect for Deno. Specifically because I think the semantics of custom inspect align with that.

The name conflict is a problem, but it can only be mitigated if it the key is to work on both platforms. If "denoCustomInspect" is practically not good enough, I'm also okay with Symbol.for("denoCustomInspect") or adding underscores or whatever.

@lucacasonato
Copy link
Member

I share the concern that @nayeemrmn brings up. It is unfortunate if modules require a Deno namespace polyfill, just because they want to enhance the experience for Deno users. This is not really great progressive enhancement.

What about Symbol.for("Deno.customInspect")? This would be non polluting, would downgrade gracefully for web, and would still maintain the esthetic of the Deno global.

@kitsonk
Copy link
Contributor

kitsonk commented Jan 28, 2021

What about Symbol.for("Deno.customInspect")?

👍

@lucacasonato lucacasonato added feat new feature (which has been agreed to/accepted) maybe 2.0 a potential feature for Deno 2.0 that needs further discussion runtime Relates to code in the runtime crate labels Jan 28, 2021
@lucacasonato
Copy link
Member

I think we can add this in the next minor release (1.8) and add a @deprecate to Deno.customInspect, and then remove it for 2.0. Should be trivial to implement.

@nayeemrmn
Copy link
Collaborator Author

Closed in #10035 (went with Symbol.for("Deno.customInspect") instead of "denoCustomInspect").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat new feature (which has been agreed to/accepted) maybe 2.0 a potential feature for Deno 2.0 that needs further discussion runtime Relates to code in the runtime crate
Projects
None yet
Development

No branches or pull requests

3 participants