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 Support in Rust Compiler #33702

Merged
merged 21 commits into from Feb 1, 2022
Merged

Conversation

ijjk
Copy link
Member

@ijjk ijjk commented Jan 26, 2022

Reverts #33699

This re-opens the support for relay in swc, although we need to narrow in the causes for the build failures in https://github.com/vercel/next.js/runs/4950448889?check_suite_focus=true

@alunyov
Copy link
Contributor

@alunyov alunyov commented Jan 26, 2022

I think there are some additional features needs to be enable for musl builds. We've added this here: https://github.com/facebook/relay/pull/3754/files

@tbezman
Copy link
Contributor

@tbezman tbezman commented Jan 26, 2022

@alunyov Can you give me the ELI5 of what went wrong here? Anything I can do to help?

@alunyov
Copy link
Contributor

@alunyov alunyov commented Jan 27, 2022

@alunyov Can you give me the ELI5 of what went wrong here? Anything I can do to help?

In relay-compiler we have a dependency on this internal crate:

https://github.com/facebook/relay/blob/c2588c778fc210b4054f4fe2cc523612b4058e90/compiler/crates/relay-compiler/Cargo.toml#L45

Which depends on hyper-tls: https://github.com/facebook/relay/blob/c2588c778fc210b4054f4fe2cc523612b4058e90/compiler/crates/persist-query/Cargo.toml#L11

We've recently added support for musl builds here: facebook/relay@f1435c3 (without it the build was also failing).

I've proposed similar changes here too (with these vendored features), will see, if they any good.

I think the real solution would be is to expose FileCategorizer, etc... from a standalone crate (and not from relay-compiler so you don't have to pull all the dependencies of relay-compiler and these feature flags won't be needed.

@alunyov
Copy link
Contributor

@alunyov alunyov commented Jan 27, 2022

... and it looks like these feature flags didn't help. https://github.com/vercel/next.js/runs/4969867563?check_suite_focus=true

I'll try to change this on our side, so we don't need to depend on the relay-compiler.

@ijjk ijjk force-pushed the revert-33699-revert-33240-relay-plugin branch from f8f7652 to 9462285 Compare Jan 27, 2022
@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@alunyov
Copy link
Contributor

@alunyov alunyov commented Jan 27, 2022

A quick update on this. It doesn't look too easy to untangle this on our (Relay) side.

Currently, this PR relies on a few things from the relay-compiler: Config and FileCategorizer.
Moving config to a different crate (relay-config) won't really help us here, as this Config would now need to depend on the persist-query (which has this hyper-tls thing).

I see two options:

  • Figure out all build issues here for Linux. Also, it looks like window builds are broken also - unable to install relay-compiler.
  • Re-implement parts of the Relay Config/ProjectConfig loading and file finder here (I think it was an initial version of this PR, before I jumped in and suggested to use relay-compiler :-)).

Let me know what do you think @tbezman @ijjk ?

@tbezman
Copy link
Contributor

@tbezman tbezman commented Jan 28, 2022

@alunyov I'll defer to you here. I not very knowledgable on this multi target build stuff. Some quick googling makes me think we would just have to install libssl-dev or something but that's probably naive. Happy to put up the same PR without the goodies.

Not sure why config has to depend on the persist-query crate. Sounds like there's some potential untangling to do to support this effort.

Your call @alunyov

kdy1
kdy1 previously requested changes Feb 1, 2022
packages/next-swc/Cargo.lock Outdated Show resolved Hide resolved
packages/next-swc/Cargo.lock Outdated Show resolved Hide resolved
@ijjk

This comment has been minimized.

@ijjk ijjk requested a review from kdy1 Feb 1, 2022
@ijjk

This comment has been minimized.

@ijjk ijjk dismissed kdy1’s stale review Feb 1, 2022

suggestions applied

@ijjk
Copy link
Member Author

@ijjk ijjk commented Feb 1, 2022

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary vercel/next.js revert-33699-revert-33240-relay-plugin Change
buildDuration 16.9s 17.1s ⚠️ +167ms
buildDurationCached 4.3s 4.3s ⚠️ +56ms
nodeModulesSize 359 MB 359 MB ⚠️ +608 B
Page Load Tests Overall increase ✓
vercel/next.js canary vercel/next.js revert-33699-revert-33240-relay-plugin Change
/ failed reqs 0 0
/ total time (seconds) 4.18 4.13 -0.05
/ avg req/sec 598.03 605.27 +7.24
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 2.143 2.123 -0.02
/error-in-render avg req/sec 1166.82 1177.52 +10.7
Client Bundles (main, webpack, commons)
vercel/next.js canary vercel/next.js revert-33699-revert-33240-relay-plugin Change
450.HASH.js gzip 179 B 179 B
framework-HASH.js gzip 42.2 kB 42.2 kB
main-HASH.js gzip 27.2 kB 27.2 kB
webpack-HASH.js gzip 1.44 kB 1.44 kB
Overall change 71 kB 71 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js revert-33699-revert-33240-relay-plugin Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary vercel/next.js revert-33699-revert-33240-relay-plugin Change
_app-HASH.js gzip 1.37 kB 1.37 kB
_error-HASH.js gzip 194 B 194 B
amp-HASH.js gzip 312 B 312 B
css-HASH.js gzip 326 B 326 B
dynamic-HASH.js gzip 2.37 kB 2.37 kB
head-HASH.js gzip 350 B 350 B
hooks-HASH.js gzip 919 B 919 B
image-HASH.js gzip 4.94 kB 4.94 kB
index-HASH.js gzip 263 B 263 B
link-HASH.js gzip 2.19 kB 2.19 kB
routerDirect..HASH.js gzip 321 B 321 B
script-HASH.js gzip 383 B 383 B
withRouter-HASH.js gzip 318 B 318 B
85e02e95b279..7e3.css gzip 107 B 107 B
Overall change 14.4 kB 14.4 kB
Client Build Manifests
vercel/next.js canary vercel/next.js revert-33699-revert-33240-relay-plugin Change
_buildManifest.js gzip 459 B 459 B
Overall change 459 B 459 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js revert-33699-revert-33240-relay-plugin Change
index.html gzip 530 B 530 B
link.html gzip 543 B 543 B
withRouter.html gzip 525 B 525 B
Overall change 1.6 kB 1.6 kB

Default Build with SWC (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary vercel/next.js revert-33699-revert-33240-relay-plugin Change
buildDuration 21.6s 21.5s -109ms
buildDurationCached 4.3s 4.3s -5ms
nodeModulesSize 359 MB 359 MB ⚠️ +608 B
Page Load Tests Overall increase ✓
vercel/next.js canary vercel/next.js revert-33699-revert-33240-relay-plugin Change
/ failed reqs 0 0
/ total time (seconds) 4.121 4.233 ⚠️ +0.11
/ avg req/sec 606.62 590.67 ⚠️ -15.95
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 2.22 2.093 -0.13
/error-in-render avg req/sec 1125.99 1194.68 +68.69
Client Bundles (main, webpack, commons)
vercel/next.js canary vercel/next.js revert-33699-revert-33240-relay-plugin Change
450.HASH.js gzip 179 B 179 B
framework-HASH.js gzip 42.3 kB 42.3 kB
main-HASH.js gzip 27.3 kB 27.3 kB
webpack-HASH.js gzip 1.44 kB 1.44 kB
Overall change 71.3 kB 71.3 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js revert-33699-revert-33240-relay-plugin Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary vercel/next.js revert-33699-revert-33240-relay-plugin Change
_app-HASH.js gzip 1.35 kB 1.35 kB
_error-HASH.js gzip 180 B 180 B
amp-HASH.js gzip 305 B 305 B
css-HASH.js gzip 321 B 321 B
dynamic-HASH.js gzip 2.36 kB 2.36 kB
head-HASH.js gzip 342 B 342 B
hooks-HASH.js gzip 911 B 911 B
image-HASH.js gzip 4.98 kB 4.98 kB
index-HASH.js gzip 256 B 256 B
link-HASH.js gzip 2.21 kB 2.21 kB
routerDirect..HASH.js gzip 314 B 314 B
script-HASH.js gzip 375 B 375 B
withRouter-HASH.js gzip 309 B 309 B
85e02e95b279..7e3.css gzip 107 B 107 B
Overall change 14.3 kB 14.3 kB
Client Build Manifests
vercel/next.js canary vercel/next.js revert-33699-revert-33240-relay-plugin Change
_buildManifest.js gzip 458 B 458 B
Overall change 458 B 458 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js revert-33699-revert-33240-relay-plugin Change
index.html gzip 530 B 530 B
link.html gzip 543 B 543 B
withRouter.html gzip 525 B 525 B
Overall change 1.6 kB 1.6 kB
Commit: cd6783a

@kodiakhq kodiakhq bot merged commit 3e60c23 into canary Feb 1, 2022
40 of 42 checks passed
@kodiakhq kodiakhq bot deleted the revert-33699-revert-33240-relay-plugin branch Feb 1, 2022
@hanford
Copy link
Contributor

@hanford hanford commented Feb 1, 2022

woot!

@ijjk
Copy link
Member Author

@ijjk ijjk commented Feb 1, 2022

Hi, this is now available in v12.0.11-canary.0 of Next.js

fn build_require_expr_from_path(path: &str) -> Expr {
Expr::Call(CallExpr {
span: Default::default(),
callee: quote_ident!("require").as_callee(),
Copy link

@maraisr maraisr Feb 1, 2022

Choose a reason for hiding this comment

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

The compiler supports eager esm as well, probably worth also exposing this.

@jcolakk
Copy link

@jcolakk jcolakk commented Feb 1, 2022

Just pulled down this version and got it working locally with my monorepo/pnpm workspace setup. Can't wait for this to land!!

@hanford any details on how you made this work in a monorepo? I'm struggling to make it work as it is requesting artifactDirectory to be set (which I really don't want), does that mean that we can't colocate __generated__ with our files but we must use artifactDirectory?

@tbezman
Copy link
Contributor

@tbezman tbezman commented Feb 2, 2022

Just pulled down this version and got it working locally with my monorepo/pnpm workspace setup. Can't wait for this to land!!

@hanford any details on how you made this work in a monorepo? I'm struggling to make it work as it is requesting artifactDirectory to be set (which I really don't want), does that mean that we can't colocate __generated__ with our files but we must use artifactDirectory?

@jcolakk In Next.js the generated code cannot be co-located since pages/__generated/GeneratedQuery.graphql.ts would add an extra route that we definitely don't want.

@maraisr
Copy link

@maraisr maraisr commented Feb 2, 2022

I've been using relay with next for about 4 years now, and use this strategy to cope with the generated files collocated.

https://dev.to/marais/scene-based-applications-ing

@alunyov
Copy link
Contributor

@alunyov alunyov commented Feb 2, 2022

@maraisr I think we can try adding support for configs without artifactDirectory in next.js. We’ll just warn if the path has ‘pages’ in it.

@tbezman
Copy link
Contributor

@tbezman tbezman commented Feb 2, 2022

@alunyov any updated thoughts on if we can massage relay source so we can add that functionality back?

@alunyov
Copy link
Contributor

@alunyov alunyov commented Feb 2, 2022

any updated thoughts on if we can massage relay source so we can add that functionality back?

@tbezman Yes, I think we can add support for projects without artifactDirectory, just throw if the query in the pages. I've added PR for this.

@alunyov alunyov mentioned this pull request Feb 2, 2022
1 task
natew pushed a commit to natew/next.js that referenced this issue Feb 16, 2022
Reverts vercel#33699

This re-opens the support for relay in swc, although we need to narrow in the causes for the build failures in https://github.com/vercel/next.js/runs/4950448889?check_suite_focus=true

Co-authored-by: Andrey Lunyov <102968+alunyov@users.noreply.github.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants