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

[relay-compiler] @preloadable Support #4515

Closed
wants to merge 20 commits into from

Conversation

tobias-tengler
Copy link
Contributor

@tobias-tengler tobias-tengler commented Nov 5, 2023

This is based on an earlier implementation done by @tomgasson in #4110

Companion PR for @types/relay-runtime: DefinitelyTyped/DefinitelyTyped#68144

An example application using this can be found here

Copy link
Contributor

@alunyov alunyov left a comment

Choose a reason for hiding this comment

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

Thank you for working on this!

compiler/crates/relay-compiler/src/config.rs Outdated Show resolved Hide resolved
@tobias-tengler tobias-tengler marked this pull request as ready for review November 6, 2023 19:00
@tobias-tengler
Copy link
Contributor Author

@alunyov this would be ready to go from my side.

I'd like to add tests, but it seems like there is no existing test setup for generate_extra_artifacts. Is there something that could be pulled into OSS from the internal Relay version or what would be the best way to go about writing tests in your opinion?

@tobias-tengler
Copy link
Contributor Author

@alunyov would you mind taking another look at this? It would be incredibly valuable for us to have this feature :)

@alunyov
Copy link
Contributor

alunyov commented Jan 5, 2024

@tobias-tengler sorry for the delay. I'll try to get to this next week

Copy link
Contributor

@alunyov alunyov left a comment

Choose a reason for hiding this comment

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

Overall, a very clean PR! Thank you for adding this.

Can you add a test for this? I think we have a way to add integration tests now: https://github.com/facebook/relay/tree/main/compiler/crates/relay-compiler/tests/relay_compiler_integration

@tobias-tengler
Copy link
Contributor Author

@alunyov Added an integration test for each language option and fixed the eagerEsModules handling. Could you kindly give it another look? :)

Copy link
Contributor

@alunyov alunyov left a comment

Choose a reason for hiding this comment

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

This looks great!

@alunyov
Copy link
Contributor

alunyov commented Jan 8, 2024

@tobias-tengler lets fix these rust formatting issues, and I'll import this internally.

@tobias-tengler
Copy link
Contributor Author

tobias-tengler commented Jan 8, 2024

@alunyov fixed :) It's weird that my editor doesn't format correctly. I'm using VS Code, rust-analyzer and I'm working from within the compiler directory. Normally the editor should format this correctly or am I missing something? I often had to fix formatting by hand with this project...
The formatting job in the CI is using a (different) nightly version of Rust than the build. I added a rustup override for 1.72.0 for the compiler as that's what's being used to build the compiler in CI. With the nightly version from the lint job I can't build the project unfortunately.

@tobias-tengler
Copy link
Contributor Author

@alunyov
Copy link
Contributor

alunyov commented Jan 8, 2024

Do you have an idea why this fails? https://github.com/facebook/relay/actions/runs/7452983745/job/20277398152?pr=4515

Did you run https://github.com/facebook/relay/blob/main/scripts/update-fixtures.sh to generate the fixture tests?

@alunyov
Copy link
Contributor

alunyov commented Jan 8, 2024

@alunyov fixed :) It's weird that my editor doesn't format correctly. I'm using VS Code, rust-analyzer and I'm working from within the compiler directory. Normally the editor should format this correctly or am I missing something? I often had to fix formatting by hand with this project... The formatting job in the CI is using a (different) nightly version of Rust than the build. I added a rustup override for 1.72.0 for the compiler as that's what's being used to build the compiler in CI. With the nightly version from the lint job I can't build the project unfortunately.

Sorry about that... We have a weird internal rust configuration, that we somehow is trying to replicate in OSS.

cc @captbaritone - were there any discussion with internal Rust team on how to properly setup OSS rust projects that needs to be synced internally?

@tobias-tengler
Copy link
Contributor Author

Oh f*** me 😂 I totally wrote them by hand, whoops.

@facebook-github-bot
Copy link
Contributor

@alunyov has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

}

//- operations.json
{}
Copy link
Contributor

Choose a reason for hiding this comment

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

@tobias-tengler somehow the query text is missing in the operations - this is maybe related to the way the integration test prints these files...

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 input. The local persister throws if the file doesn't already exists. This is just here to satisfy the persister.
Screenshot 2024-01-08 at 22 23 20

@alunyov
Copy link
Contributor

alunyov commented Jan 9, 2024

@tobias-tengler you'll also need to update integration test setup, to include the default extra generator to the test config

@facebook-github-bot
Copy link
Contributor

@alunyov has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@alunyov merged this pull request in 40fe615.

@alex-statsig
Copy link
Contributor

Can / should this support mutations?

I've noticed mutations often add up to a large chunk of bundle sizes (sometimes due to including ...EntirePage to ensure data is all refetched). It seems like the AST shouldn't be needed immediately for mutations, and committing the mutation with appropriate variables could be decoupled (especially with persisted queries removing the need for the entire query text in the bundle).

It's not quite the same flow as preloading queries, but a similar idea I think to prioritize the part needed to issue the request (and defer the part to process the response). It seems especially applicable since mutations are not used more often than they are used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants