Conversation
sandersn
approved these changes
Mar 6, 2024
Member
sandersn
left a comment
There was a problem hiding this comment.
I like this idea, and would love for it to include a way to remember to update lib/esnext.*.d.ts. (Right now I think our intended approach is to manually check each year after the June TC39 meeting.)
jakebailey
reviewed
Mar 6, 2024
| * | ||
| * @internal | ||
| */ | ||
| export namespace LanguageFeatureMinimumTarget { |
Member
There was a problem hiding this comment.
Can we turn this into a const enum such that it gets inlined? I believe we should be able to reference ScriptTarget from one. Or does that fail because of the type mismatch with ScriptTarget?
(Would like to limit our use of namespaces, honestly... plus potential perf hits here)
Member
There was a problem hiding this comment.
Or just a big const since we don't really need the reverse lookup (perf hit should be almost none)
Contributor
Author
There was a problem hiding this comment.
It doesn't fail to match ScriptTarget, but it does produce its own type which doesn't illustrate the relationship when looking at the .d.ts file:
vs.
vs.
I actually still like namespace in moderation. This felt like an appropriate use for it, but I don't mind the as const. There's no perf difference between namespace and as const.
Member
There was a problem hiding this comment.
This is an internal declaration, so I don't personally feel like it's a big deal for the values to be opaque. It does make me wonder if we're missing something in declaration emit that would have reused those nodes, though.
Contributor
Author
There was a problem hiding this comment.
Namespaces, as a concept, exist in both the standard library (Math, Atomics, Reflect) and the web platform (subtle, performance, etc.) so I don't agree that they should be wholly discouraged.
Member
There was a problem hiding this comment.
Sure, I'm just trying to avoid the runtime cost of them in the code we do control (e.g. const enums are inlined, modules are tree shaken).
Contributor
Author
There was a problem hiding this comment.
If the perf results show a significant dip I can make it a const enum. It just feels less useful when hover information only shows the final value like 2 instead of something meaningful like ES2015.
Member
There was a problem hiding this comment.
I see what you mean; I do wish our hover and declaration emit did better here. But, we seem to just always compute in dts emit (not sure about hover).
Contributor
Author
There was a problem hiding this comment.
I see what you mean; I do wish our hover and declaration emit did better here. But, we seem to just always compute in dts emit (not sure about hover).
It's arguable as to whether someone who writes const enum E2 { A = E1.B } would expect hover/d.ts to print the value or qualified name. With const variables or fields it's far clearer as to what to show. I could see maybe supporting const enum E2 { A = E1.B as E1.B } or something similar to explicitly opt-in to showing the qualified name, but that seems like a very niche use case.
Member
There was a problem hiding this comment.
I would much rather see (enum member) TypeFlags.Unit = Enum | Literal | UniqueESSymbol | Nullable than (enum member) TypeFlags.Unit = 109472. The actual computed values seem like the least important part of an enum.
Member
|
This is great |
Contributor
Author
The best we might be able to do is to just document this, |
Contributor
Author
|
@typescript-bot perf test |
Collaborator
Collaborator
|
@rbuckton Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Contributor
Author
|
@typescript-bot perf test |
Collaborator
Collaborator
|
@rbuckton Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Contributor
Author
|
It looks like the |
jakebailey
approved these changes
Mar 7, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds
LanguageFeatureMinimumTarget, which is used to track the specific ECMAScript edition that various language features were standardized. This is intended to be a central location to check for when specific features are added to the language and provide a single place to update a feature's minimum target fromESNextto the edition it landed in.In keeping with this, all of our emit helper checks have been tightened up to reference the correct version for when a given emit helper is not necessary during emit.
Fixes #41241