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 Deno.customInspect symbol at 2.0 #10929

Open
kt3k opened this issue Jun 11, 2021 · 4 comments
Open

Remove Deno.customInspect symbol at 2.0 #10929

kt3k opened this issue Jun 11, 2021 · 4 comments
Assignees
Labels
breaking change a change or feature that breaks existing semantics feat new feature (which has been agreed to/accepted) maybe 2.0 a potential feature for Deno 2.0 that needs further discussion
Milestone

Comments

@kt3k
Copy link
Member

kt3k commented Jun 11, 2021

We're going to deprecate Deno.customInspect symbol in favor of Symbol.for("Deno.customInspect") in #10035

This issue is for tracking the future removal of Deno.customInspect.

@kt3k kt3k added breaking change a change or feature that breaks existing semantics maybe 2.0 a potential feature for Deno 2.0 that needs further discussion labels Jun 11, 2021
@stale
Copy link

stale bot commented Aug 10, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 10, 2021
@kitsonk kitsonk added the feat new feature (which has been agreed to/accepted) label Aug 10, 2021
@stale stale bot removed the stale label Aug 10, 2021
@kitsonk kitsonk mentioned this issue Sep 17, 2021
17 tasks
@kitsonk kitsonk added this to the 2.0.0 milestone Sep 17, 2021
@kitsonk kitsonk added this to To do in Deno CLI 2.0 Sep 17, 2021
@nayeemrmn
Copy link
Collaborator

nayeemrmn commented Sep 28, 2021

We're going to deprecate Deno.customInspect symbol in favor of Symbol.for("Deno.customInspect")

@kt3k I want to revive the idea of using just "denoCustomInspect" for the key instead. This was my original proposal in #9294 but I didn't push for it at the time. However it came up on discord and others also found the current pattern strange: https://discord.com/channels/684898665143206084/688040863313428503/868555515729436692, https://discord.com/channels/684898665143206084/688040863313428503/868628683227824198.

Symbol.for("Deno.customInspect") was an illogical compromise between unique symbol which is non-polluting but non-compatible, and a string convention which is cross-compatible but polluting. The pollution is a mainly theoretical concern either way which would affect any compatible solution including Symbol.for("Deno.customInspect"). A string convention is far more traditional and has less friction.

"Deno.customInspect" instead of "denoCustomInspect" is also an option, though personally I don't think the Deno namespace aesthetic is important enough to warrant requiring quotes to declare it.

@kt3k
Copy link
Member Author

kt3k commented Sep 29, 2021

I think non unique symbols are still less polluting than string keys because symbol keys are always non enumerable even when it is set to the object directly. If we set "denoCustomInspect" directly to the object, that appears in enumeration, and that can be annoying.

It seems a common pattern recently to use symbols for some special protocols eg. Symbol.iterator, Symbol.asyncIterator. So using symbol here express better the idea that this is a special protocol, not a usual function. So I prefer to keep this as a symbol.

@nayeemrmn
Copy link
Collaborator

I think non unique symbols are still less polluting than string keys because symbol keys are always non enumerable even when it is set to the object directly. If we set "denoCustomInspect" directly to the object, that appears in enumeration, and that can be annoying.

Most of the time it will be used on a class though. Even if it was set directly on an object and was enumerable, I'm not convinced that's ever a problem. It would only be as bad as implementing .toString(). I think when users are concerned about enumerability to this extent, they are deliberate and use Object.defineProperties().

It seems a common pattern recently to use symbols for some special protocols eg. Symbol.iterator, Symbol.asyncIterator. So using symbol here express better the idea that this is a special protocol, not a usual function.

These are actually unique symbols whose references are built into the language. They are more like Deno.customInspect but don't have a compatibility problem. They act as protocol implementations because they are unique. Symbol.for("Deno.customInspect") doesn't fulfil that purpose or convey that meaning, the feedback I've seen is confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change a change or feature that breaks existing semantics feat new feature (which has been agreed to/accepted) maybe 2.0 a potential feature for Deno 2.0 that needs further discussion
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants