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

Upgrade Javet, node, V8, tsc and node-gradle #70

Merged
merged 4 commits into from
Nov 3, 2023

Conversation

ammachado
Copy link
Contributor

What's changed?

Javet, node, V8 tsc and node-gradle have been updated

What's your motivation?

Running mod on Apache Camel was taking almost 1h. By observation on the progress bar, parsing of javascript files was slow, so I'm updating this project to test later

Anything in particular you'd like reviewers to focus on?

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

Any additional context

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've added the license header to any new files through ./gradlew licenseFormat
  • I've used the IntelliJ IDEA auto-formatter on affected files

@ammachado ammachado force-pushed the runtime-upgrade branch 2 times, most recently from 2d5dc34 to 2d068a5 Compare October 20, 2023 05:26
@timtebeek timtebeek added enhancement New feature or request dependencies Pull requests that update a dependency file labels Oct 20, 2023
@timtebeek
Copy link
Contributor

Thanks for putting in the effort in here @ammachado ; had indeed noticed that parsing web resources takes a while on various projects, hope this helps speed that up. I'm not the best judge of the changes here, so I'm hoping @traceyyoshima is able to have a look.

@ammachado ammachado removed their assignment Oct 21, 2023
@ammachado
Copy link
Contributor Author

@timtebeek @traceyyoshima I added a test to reproduce the slowness.

@knutwannheden
Copy link
Contributor

@ammachado Thanks for your work and your performance test. As you may have noticed this is still work in progress and not all syntactic constructs are supported yet. I did however commit some improvements now which should improve performance and also adds support for some constructs (e.g. the delete expression), which weren't supported before.

Copy link
Contributor

@traceyyoshima traceyyoshima left a comment

Choose a reason for hiding this comment

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

Hi @ammachado, Thank you for the PR, and my apologies for taking so long to review the changes! Changes look good -- we'll make a few updates on our end so the build will pass. There was an API change, and a test fails due to an existing parsing issue unrelated to the PR>

@traceyyoshima traceyyoshima merged commit 678c5e0 into openrewrite:main Nov 3, 2023
1 of 2 checks passed
@ammachado
Copy link
Contributor Author

@traceyyoshima I'm happy to help.

@ammachado ammachado deleted the runtime-upgrade branch November 5, 2023 04:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants