Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

Feat(rome_js_parser): typeof private property name #2524

Conversation

IWANABETHATGUY
Copy link
Contributor

Summary

part of #2400
typeof on private fields: Announcement

Test Plan

I copy the test case from official pull request https://github.com/microsoft/TypeScript/pull/47696/files#diff-c7d8064feb0c4b5ba2cb75b7987eefc92a880fded5f86a9564f5d30ffef48bff, and pass it.

@IWANABETHATGUY IWANABETHATGUY changed the title Feat/rome js parser typeof private property name Feat(rome_js_parser): typeof private property name May 2, 2022
@IWANABETHATGUY
Copy link
Contributor Author

!bench_parser

@github-actions
Copy link

github-actions bot commented May 2, 2022

Parser Benchmark Results

group                                 main                                   pr
-----                                 ----                                   --
parser/checker.ts                     1.03    123.7±4.55ms    21.0 MB/sec    1.00    119.7±1.46ms    21.7 MB/sec
parser/compiler.js                    1.00     72.5±1.44ms    14.4 MB/sec    1.01     73.1±1.44ms    14.3 MB/sec
parser/d3.min.js                      1.00     46.6±0.44ms     5.6 MB/sec    1.00     46.4±0.28ms     5.7 MB/sec
parser/dojo.js                        1.00      4.4±0.00ms    15.8 MB/sec    1.00      4.4±0.00ms    15.7 MB/sec
parser/ios.d.ts                       1.01    105.1±1.61ms    17.8 MB/sec    1.00    104.1±0.86ms    17.9 MB/sec
parser/jquery.min.js                  1.00     12.7±0.02ms     6.5 MB/sec    1.00     12.8±0.03ms     6.5 MB/sec
parser/math.js                        1.02     88.8±1.46ms     7.3 MB/sec    1.00     87.0±1.07ms     7.4 MB/sec
parser/parser.ts                      1.00      3.0±0.00ms    16.0 MB/sec    1.00      3.0±0.00ms    16.0 MB/sec
parser/pixi.min.js                    1.01     55.3±0.72ms     7.9 MB/sec    1.00     54.6±0.58ms     8.0 MB/sec
parser/react-dom.production.min.js    1.00     17.5±0.03ms     6.6 MB/sec    1.00     17.6±0.03ms     6.6 MB/sec
parser/react.production.min.js        1.00    921.0±2.11µs     6.7 MB/sec    1.00    920.2±1.98µs     6.7 MB/sec
parser/router.ts                      1.00      2.5±0.01ms    24.1 MB/sec    1.00      2.5±0.00ms    24.1 MB/sec
parser/tex-chtml-full.js              1.01    116.1±1.11ms     7.8 MB/sec    1.00    114.4±1.26ms     8.0 MB/sec
parser/three.min.js                   1.02     62.9±0.89ms     9.3 MB/sec    1.00     61.7±0.39ms     9.5 MB/sec
parser/typescript.js                  1.02    491.8±7.43ms    19.3 MB/sec    1.00    480.6±8.04ms    19.8 MB/sec
parser/vue.global.prod.js             1.00     21.3±0.15ms     5.7 MB/sec    1.00     21.4±0.09ms     5.6 MB/sec

@IWANABETHATGUY
Copy link
Contributor Author

@MichaReiser could you help with reviewing my pull request about the new typescript 4.7 syntax?

@@ -2129,7 +2129,7 @@ TsAnyName =
TsQualifiedName =
left: TsAnyName
'.'
right: JsName
right: JsAnyName
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having concerns about adding JsAnyName to the right-hand side. This will not only allow private names in typeof expressions but anywhere else where TsQualifiedName is used (and none of these allow private names).

Can you go through all types that have a field with the type TsQualifiedName and check if TS now allows the usage of private names for these use cases? For example, TsImportTypeQualifier, TsNameWithTypeArguments, TsReferenceType, TsAnyModuleReference

Does TS support typeof this.#a.b?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p

Ts support typeof this.#a.b , https://www.typescriptlang.org/play?#code/FAYwNghgzlAEAqsDexZtgYgrAvMiAXLAOTEC+q6ALgKZRUAUAlMpetQJ4AONA9gGawqACwCWUAHRYJEANxtYFMkA, but I don't have enough knowledge about Ts specification, so I can't write code snippet include the ast node you mentioned above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to understand the exact change of typescript to model the AST correctly.

Are you able to find the related PR in the TypeScript repository?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading the code of this PR should help you understand which rules allow private names and which do not (allowPrivateIdentifiers parameter).

The next step is then to go to TypeScript's playground and try to come up with examples for the cases where private names are/aren't allowed.

@jakebailey
Copy link

We're reverting the linked change in TS, so you probably shouldn't merge this (we may not even allow this syntax).

@MichaReiser
Copy link
Contributor

We're reverting the linked change in TS, so you probably shouldn't merge this (we may not even allow this syntax).

Thanks @jakebailey for letting us know!

@IWANABETHATGUY IWANABETHATGUY deleted the feat/rome_js_parser_typeof_private_property_name branch September 12, 2022 12:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants