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

More fluent GraphTraversal API #4598

Merged
merged 1 commit into from
May 1, 2023
Merged

More fluent GraphTraversal API #4598

merged 1 commit into from
May 1, 2023

Conversation

alexkirsz
Copy link
Contributor

@alexkirsz alexkirsz commented Apr 17, 2023

Description

This PR refactors the API of GraphTraversal to be more fluent, and less magical. It should make it easier to use and understand. Had this in store from trying to debug a lifetime issue in another branch.

Testing Instructions

Surface-level change.

Next.js PR: https://github.com/vercel/turbo/pulls/alexkirsz

@alexkirsz alexkirsz requested a review from a team as a code owner April 17, 2023 08:28
@vercel
Copy link

vercel bot commented Apr 17, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
examples-basic-web 🔄 Building (Inspect) May 1, 2023 8:35am
examples-cra-web 🔄 Building (Inspect) May 1, 2023 8:35am
examples-nonmonorepo 🔄 Building (Inspect) May 1, 2023 8:35am
examples-tailwind-web 🔄 Building (Inspect) May 1, 2023 8:35am
turbo-site ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 1, 2023 8:35am
6 Ignored Deployments
Name Status Preview Comments Updated (UTC)
examples-designsystem-docs ⬜️ Ignored (Inspect) May 1, 2023 8:35am
examples-gatsby-web ⬜️ Ignored (Inspect) May 1, 2023 8:35am
examples-kitchensink-blog ⬜️ Ignored (Inspect) May 1, 2023 8:35am
examples-native-web ⬜️ Ignored (Inspect) May 1, 2023 8:35am
examples-svelte-web ⬜️ Ignored (Inspect) May 1, 2023 8:35am
examples-vite-web ⬜️ Ignored (Inspect) May 1, 2023 8:35am

@alexkirsz alexkirsz changed the title Graph refactor More fluent GraphTraversal API Apr 17, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Apr 17, 2023

✅ This change can build next-swc

@github-actions
Copy link
Contributor

github-actions bot commented Apr 17, 2023

🟢 CI successful 🟢

Thanks

@github-actions
Copy link
Contributor

Benchmark for ce22e68

Click to view benchmark
Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 8975.22µs ± 54.89µs 8994.08µs ± 43.47µs +0.21%
bench_hmr_to_eval/Turbopack CSR/1000 modules 8051.40µs ± 55.16µs 8051.46µs ± 36.72µs +0.00%
bench_startup/Turbopack CSR/1000 modules 868.76ms ± 1.27ms 867.31ms ± 1.12ms -0.17%

Comment on lines +23 to +25
let completions = NonDeterministic::new()
.skip_duplicates()
.visit([root], get_referenced_assets)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the difference in interface: no more type wrangling to create the traversal.

@@ -66,9 +90,9 @@ where
}
}
Copy link
Member

Choose a reason for hiding this comment

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

SkipDuplicates could implement skip_duplicates and return self

Copy link
Contributor Author

@alexkirsz alexkirsz May 1, 2023

Choose a reason for hiding this comment

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

With the way the traits are set up, this would require specialization, so I'm going to skip this for now.

.await
.completed()?
.into_inner();
let graph = ReverseTopological::<Type>::new()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let graph = ReverseTopological::<Type>::new()
let graph = ReverseTopological::new()

It should be able to infer that, shouldn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try!

@alexkirsz alexkirsz force-pushed the alexkirsz/graph-refactor branch from de35f0e to b9cfb3b Compare May 1, 2023 08:34
@alexkirsz alexkirsz added the pr: automerge Kodiak will merge these automatically after checks pass label May 1, 2023
@kodiakhq kodiakhq bot merged commit 161aa10 into main May 1, 2023
@kodiakhq kodiakhq bot deleted the alexkirsz/graph-refactor branch May 1, 2023 08:48
sokra added a commit to vercel/next.js that referenced this pull request May 1, 2023
See vercel/turborepo#4598

### Turbopack changes

* vercel/turborepo#4754 <!-- Justin Ridgewell -
Match TS's extends resolution algorithm -->
* vercel/turborepo#4598 <!-- Alex Kirszenberg - More
fluent GraphTraversal API -->

---------

Co-authored-by: Tobias Koppers <tobias.koppers@googlemail.com>
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 25, 2024
### Description

This PR refactors the API of `GraphTraversal` to be more fluent, and
less magical. It should make it easier to use and understand. Had this
in store from trying to debug a lifetime issue in another branch.

### Testing Instructions

Surface-level change.

Next.js PR: https://github.com/vercel/turbo/pulls/alexkirsz
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 29, 2024
### Description

This PR refactors the API of `GraphTraversal` to be more fluent, and
less magical. It should make it easier to use and understand. Had this
in store from trying to debug a lifetime issue in another branch.

### Testing Instructions

Surface-level change.

Next.js PR: https://github.com/vercel/turbo/pulls/alexkirsz
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Aug 1, 2024
### Description

This PR refactors the API of `GraphTraversal` to be more fluent, and
less magical. It should make it easier to use and understand. Had this
in store from trying to debug a lifetime issue in another branch.

### Testing Instructions

Surface-level change.

Next.js PR: https://github.com/vercel/turbo/pulls/alexkirsz
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: automerge Kodiak will merge these automatically after checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants