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

feat(rome_js_parser): optional variance annotation #2556

Conversation

IWANABETHATGUY
Copy link
Contributor

@IWANABETHATGUY IWANABETHATGUY commented May 8, 2022

Summary

part of #2400

current:
File Based Average Prettier Similarity: 72.69%
Line Based Average Prettier Similarity: 67.25%

main:
File Based Average Prettier Similarity: 72.65%
Line Based Average Prettier Similarity: 67.13%

Test Plan

I took test cases from esbuild evanw/esbuild#2102, the original test case of Typescript pull request related to semantics, which we don't care too much about. esbuild Optional variance annotations pull request test case is more suitable for us.

@IWANABETHATGUY IWANABETHATGUY changed the title Feat/optional variance annotation Feat(rome_js_parser):optional variance annotation May 8, 2022
@IWANABETHATGUY
Copy link
Contributor Author

!bench_parser

@github-actions
Copy link

github-actions bot commented May 8, 2022

Parser Benchmark Results

group                                 main                                   pr
-----                                 ----                                   --
parser/checker.ts                     1.12    136.6±2.57ms    19.0 MB/sec    1.00    122.3±2.32ms    21.2 MB/sec
parser/compiler.js                    1.00     70.3±1.38ms    14.9 MB/sec    1.11     78.3±3.85ms    13.4 MB/sec
parser/d3.min.js                      1.01     42.9±0.70ms     6.1 MB/sec    1.00     42.6±0.73ms     6.2 MB/sec
parser/dojo.js                        1.01      3.8±0.01ms    18.2 MB/sec    1.00      3.7±0.00ms    18.4 MB/sec
parser/ios.d.ts                       1.05    106.2±1.86ms    17.6 MB/sec    1.00    101.6±1.95ms    18.4 MB/sec
parser/jquery.min.js                  1.01     11.1±0.03ms     7.4 MB/sec    1.00     11.0±0.03ms     7.5 MB/sec
parser/math.js                        1.00     84.9±1.56ms     7.6 MB/sec    1.00     84.8±1.35ms     7.6 MB/sec
parser/parser.ts                      1.02      2.6±0.01ms    18.3 MB/sec    1.00      2.6±0.00ms    18.7 MB/sec
parser/pixi.min.js                    1.01     51.9±1.08ms     8.5 MB/sec    1.00     51.7±1.59ms     8.5 MB/sec
parser/react-dom.production.min.js    1.15     17.5±0.13ms     6.6 MB/sec    1.00     15.2±0.06ms     7.6 MB/sec
parser/react.production.min.js        1.01    834.6±1.39µs     7.4 MB/sec    1.00    826.4±0.72µs     7.4 MB/sec
parser/router.ts                      1.02      2.2±0.01ms    28.1 MB/sec    1.00      2.1±0.00ms    28.6 MB/sec
parser/tex-chtml-full.js              1.03    116.6±1.61ms     7.8 MB/sec    1.00    112.9±2.97ms     8.1 MB/sec
parser/three.min.js                   1.00     60.2±1.09ms     9.8 MB/sec    1.00     60.3±1.47ms     9.7 MB/sec
parser/typescript.js                  1.01    526.3±6.63ms    18.0 MB/sec    1.00    523.5±7.09ms    18.1 MB/sec
parser/vue.global.prod.js             1.01     19.0±0.12ms     6.3 MB/sec    1.00     18.9±0.15ms     6.4 MB/sec

@IWANABETHATGUY IWANABETHATGUY marked this pull request as ready for review May 8, 2022 16:24
@IWANABETHATGUY
Copy link
Contributor Author

This pull request is pretty big, so I prefer to separate the optional variance annotation formatting another pr

@IWANABETHATGUY
Copy link
Contributor Author

@MichaReiser , @ematipico could we make some progress here ? Since prettier has been landed Ts 4.7, prettier/prettier#12900

})
.parse_exclusive_syntax(
p,
|p| parse_ts_type_parameters(p, true),
Copy link
Contributor

Choose a reason for hiding this comment

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

I find true/false parameters hard to understand. Having a parse_ts_type_parameters_impl function that takes a boolean parameter is fine but I would probably expose a parse_ts_type_parameters and a ts_type_parameters_with_modifiers function instead to make the difference clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only thing I want to argue is that the only difference between parse_ts_type_parameters_impl and parse_ts_type_parameters would be https://github.com/IWANABETHATGUY/tools/blob/d8dd387ec11b49fe46da005c53f7ed17aa34e387/crates/rome_js_parser/src/syntax/typescript/types.rs#L110-L111, Is this duplication acceptable ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MichaReiser, any thought about this?

@ematipico ematipico changed the title Feat(rome_js_parser):optional variance annotation feat(rome_js_parser): optional variance annotation Jun 14, 2022
@ematipico ematipico added the PR: on hold A PR that needs some upstream work before getting merged. label Jun 22, 2022
@LouisStairwage LouisStairwage mentioned this pull request Dec 12, 2022
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PR: on hold A PR that needs some upstream work before getting merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants