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

[Flight] Fix reference param type in registerServerReference #27688

Merged

Conversation

unstubbable
Copy link
Collaborator

@unstubbable unstubbable commented Nov 10, 2023

The reference that is passed into registerServerReference can be a plain function. It does not need to have the three additonal properties of a ServerRefeference. In fact, adding these properties (plus bind) is precisely what registerServerReference does.

@react-sizebot
Copy link

react-sizebot commented Nov 10, 2023

Comparing: 6c7b41d...bbc6c29

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 175.90 kB 175.90 kB = 54.75 kB 54.75 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 177.97 kB 177.97 kB = 55.39 kB 55.39 kB
facebook-www/ReactDOM-prod.classic.js = 569.81 kB 569.81 kB = 100.29 kB 100.29 kB
facebook-www/ReactDOM-prod.modern.js = 553.67 kB 553.67 kB = 97.38 kB 97.38 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against bbc6c29

@unstubbable unstubbable force-pushed the fix-register-server-reference-signature branch from 28f709e to 0888c90 Compare November 15, 2023 15:51
The `reference` that is passed into `registerServerReference` can be a
plain function. It does not need to have the three additonal properties
of a `ServerRefeference`. In fact, adding these properties (plus `bind`)
is precisely what `registerServerReference` does.
@unstubbable unstubbable force-pushed the fix-register-server-reference-signature branch from 0888c90 to bbc6c29 Compare November 24, 2023 16:45
@unstubbable
Copy link
Collaborator Author

I think this is still valid, even with #28343.

@sebmarkbage
Copy link
Collaborator

It technically shouldn't even be limited to Functions. It technically should be possible to export any object from a "use server" and pass it back to the server by reference. ServerReferences probably shouldn't even have extra properties and just be a WeakMap.

@sebmarkbage
Copy link
Collaborator

There's a static typing thing here that comes into play and is not fully thought out yet. Really types should be enforced at the "use client" / "use server" boundaries so you should not be able to annotate something that is non-serializable as an argument across the boundary. This can be enforced by the list of types we know are allowed to pass.

However, function is not a type that is allowed to pass. So passing an action would be an error. Therefore these exports actually needs to get a special marker type when they're imported so that this type can make them allowed again.

@unstubbable
Copy link
Collaborator Author

unstubbable commented Feb 15, 2024

Yeah, this was more meant from a framework's/bundler's perspective. When implementing the bundling part that generates the registerServerReference calls I stumbled upon this.

@sebmarkbage sebmarkbage merged commit 9268672 into facebook:main Feb 15, 2024
36 checks passed
@unstubbable unstubbable deleted the fix-register-server-reference-signature branch February 16, 2024 09:56
huozhi added a commit to vercel/next.js that referenced this pull request Feb 23, 2024
### React upstream changes

- facebook/react#28333
- facebook/react#28334
- facebook/react#28378
- facebook/react#28377
- facebook/react#28376
- facebook/react#28338
- facebook/react#28331
- facebook/react#28336
- facebook/react#28320
- facebook/react#28317
- facebook/react#28375
- facebook/react#28367
- facebook/react#28380
- facebook/react#28368
- facebook/react#28343
- facebook/react#28355
- facebook/react#28374
- facebook/react#28362
- facebook/react#28344
- facebook/react#28339
- facebook/react#28353
- facebook/react#28346
- facebook/react#25790
- facebook/react#28352
- facebook/react#28326
- facebook/react#27688
- facebook/react#28329
- facebook/react#28332
- facebook/react#28340
- facebook/react#28327
- facebook/react#28325
- facebook/react#28324
- facebook/react#28309
- facebook/react#28310
- facebook/react#28307
- facebook/react#28306
- facebook/react#28315
- facebook/react#28318
- facebook/react#28226
- facebook/react#28308
- facebook/react#27563
- facebook/react#28297
- facebook/react#28286
- facebook/react#28284
- facebook/react#28275
- facebook/react#28145
- facebook/react#28301
- facebook/react#28224
- facebook/react#28152
- facebook/react#28296
- facebook/react#28294
- facebook/react#28279
- facebook/react#28273
- facebook/react#28269
- facebook/react#28376
- facebook/react#28338
- facebook/react#28331
- facebook/react#28336
- facebook/react#28320
- facebook/react#28317
- facebook/react#28375
- facebook/react#28367
- facebook/react#28380
- facebook/react#28368
- facebook/react#28343
- facebook/react#28355
- facebook/react#28374
- facebook/react#28362
- facebook/react#28344
- facebook/react#28339
- facebook/react#28353
- facebook/react#28346
- facebook/react#25790
- facebook/react#28352
- facebook/react#28326
- facebook/react#27688
- facebook/react#28329
- facebook/react#28332
- facebook/react#28340
- facebook/react#28327
- facebook/react#28325
- facebook/react#28324
- facebook/react#28309
- facebook/react#28310
- facebook/react#28307
- facebook/react#28306
- facebook/react#28315
- facebook/react#28318
- facebook/react#28226
- facebook/react#28308
- facebook/react#27563
- facebook/react#28297
- facebook/react#28286
- facebook/react#28284
- facebook/react#28275
- facebook/react#28145
- facebook/react#28301
- facebook/react#28224
- facebook/react#28152
- facebook/react#28296
- facebook/react#28294
- facebook/react#28279
- facebook/react#28273
- facebook/react#28269

Closes NEXT-2542


Disable ppr test for strict mode for now, @acdlite will check it and
we'll sync again
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…ebook#27688)

The `reference` that is passed into `registerServerReference` can be a
plain function. It does not need to have the three additonal properties
of a `ServerRefeference`. In fact, adding these properties (plus `bind`)
is precisely what `registerServerReference` does.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
)

The `reference` that is passed into `registerServerReference` can be a
plain function. It does not need to have the three additonal properties
of a `ServerRefeference`. In fact, adding these properties (plus `bind`)
is precisely what `registerServerReference` does.

DiffTrain build for commit 9268672.
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.

4 participants