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 recast and use ast-types directly #349

Merged
merged 8 commits into from
May 4, 2019
Merged

Remove recast and use ast-types directly #349

merged 8 commits into from
May 4, 2019

Conversation

danez
Copy link
Collaborator

@danez danez commented Apr 23, 2019

This removes recast and only uses ast-types directly.

The performance increases significantly, but I haven't tested more different cases.

There are still some cases that do not yet work:

  • Printing of nodes which were created in react-docgen by ast-types builders.
  • Comments in printed code are not filtered out

Fixes #237

@fkling
Copy link
Member

fkling commented Apr 23, 2019

Amazing. Somehow I thought the whole NodePath business was part of recast (that would probably be more difficult to remove).

Looking forward to where this goes 😃

The performance increases significantly, but I haven't tested more different cases.

Can you share numbers?

@danez
Copy link
Collaborator Author

danez commented Apr 23, 2019

yeah I forgot to include that yesterday:

Before:

❯ node --expose-gc --max-old-space-size=2000 --max-semi-space-size=1500 --noconcurrent_sweeping ./benchmark/index.js
Node: v10.15.1
Running benchmark for CircularProgress.js ...
┌─────────────────────┬─────────────────────────────┐
│ fixture             │ timing                      │
├─────────────────────┼─────────────────────────────┤
│ CircularProgress.js │ 30.34 ops/sec ±7.82% (33ms) │
└─────────────────────┴─────────────────────────────┘

With PR:

❯ node --expose-gc --max-old-space-size=2000 --max-semi-space-size=1500 --noconcurrent_sweeping ./benchmark/index.js
Node: v10.15.1
Running benchmark for CircularProgress.js ...
┌─────────────────────┬─────────────────────────────┐
│ fixture             │ timing                      │
├─────────────────────┼─────────────────────────────┤
│ CircularProgress.js │ 40.55 ops/sec ±5.08% (25ms) │
└─────────────────────┴─────────────────────────────┘

The commandline options are to prevent gc to interfere, so the script runs without automatic gc.

The test file is taken from material ui. I need to find bigger or different (flow) files to test on. Would also be interesting to run this on a big codebase and see the difference.

@fkling
Copy link
Member

fkling commented Apr 23, 2019

Would also be interesting to run this on a big codebase and see the difference.

I get the hint 😉

@danez danez marked this pull request as ready for review April 26, 2019 01:39
@danez danez changed the title WIP: Remove recast and use ast-types directly Remove recast and use ast-types directly May 2, 2019
@danez danez added this to the v5 milestone May 2, 2019
@danez danez merged commit 8fb4010 into master May 4, 2019
@danez danez deleted the no-recast branch May 4, 2019 03:50
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 2022
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.

2 participants