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

[RSC @ Meta] Simplify implementation of isClientReference, getClientReferenceKey, resolveClientReferenceMetadata #27839

Merged
merged 6 commits into from
Dec 19, 2023

Conversation

alunyov
Copy link
Contributor

@alunyov alunyov commented Dec 15, 2023

For clientReferences we can just check the instance of the clientReference.
The implementation of isClientReference is provided via configuration. The class for ClientReference has to implement an interface that has `getModuleId() method.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Dec 15, 2023
@react-sizebot
Copy link

react-sizebot commented Dec 15, 2023

Comparing: 493610f...5f125f9

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.76 kB 54.76 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 = 570.21 kB 570.21 kB = 100.35 kB 100.35 kB
facebook-www/ReactDOM-prod.modern.js = 554.06 kB 554.06 kB = 97.43 kB 97.43 kB
test_utils/ReactAllWarnings.js Deleted 67.41 kB 0.00 kB Deleted 16.49 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-www/ReactFlightDOMServer-dev.modern.js = 80.65 kB 80.38 kB = 17.14 kB 17.09 kB
facebook-www/ReactFlightDOMServer-prod.modern.js = 38.58 kB 38.23 kB = 8.67 kB 8.62 kB
test_utils/ReactAllWarnings.js Deleted 67.41 kB 0.00 kB Deleted 16.49 kB 0.00 kB

Generated by 🚫 dangerJS against 5f125f9

@sebmarkbage
Copy link
Collaborator

The register functions are really meant to move to be more unified between multiple implementations. It might be more obvious when you deal with Server References that has more of that implemented already.

Originally this was more up to each config but we realized we started add more and more features to the references themselves. For example we're trying to unify the Proxy implementation so that they can provide the same error messages when you try to access something you shouldn't and that you can create the same indirections.

Another feature is that on Server References, we extend .bind() so that you can create bound versions of the reference and pass around. E.g. curry a Server Action. So we added that to all the register methods. Arguably the .bind() feature should exist on client references too. We couldn't do that consistently if we didn't have a register Hook where React can do this.

But currently the implementation is a bit ambivalent about how strongly React needs to control the reference.

@sebmarkbage
Copy link
Collaborator

That's not to say you can't remove the registeredClientReferences map but it would be good to have the register functions around for the secondary purpose (they can be noops for now).

@alunyov
Copy link
Contributor Author

alunyov commented Dec 16, 2023

Thanks @sebmarkbage! That makes sense, I'll keep the register functions (as noop for now) to keep the public API consistent.

Comment on lines 12 to 15
// eslint-disable-next-line no-unused-vars
export type ServerReference<T> = string;

// eslint-disable-next-line no-unused-vars

Choose a reason for hiding this comment

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

are the // eslint-disable-next-line no-unused-vars actually needed?

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 just to keep the same interface across different configurations, these types are using generic T, which is unused in this specific case.

@alunyov alunyov merged commit cb24396 into facebook:main Dec 19, 2023
36 checks passed
github-actions bot pushed a commit that referenced this pull request Dec 19, 2023
…eferenceKey, resolveClientReferenceMetadata (#27839)

For clientReferences we can just check the instance of the
`clientReference`.
The implementation of `isClientReference` is provided via configuration.
The class for ClientReference has to implement an interface that has
`getModuleId() method.

DiffTrain build for [cb24396](cb24396)
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…eferenceKey, resolveClientReferenceMetadata (facebook#27839)

For clientReferences we can just check the instance of the
`clientReference`.
The implementation of `isClientReference` is provided via configuration.
The class for ClientReference has to implement an interface that has
`getModuleId() method.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
…eferenceKey, resolveClientReferenceMetadata (#27839)

For clientReferences we can just check the instance of the
`clientReference`.
The implementation of `isClientReference` is provided via configuration.
The class for ClientReference has to implement an interface that has
`getModuleId() method.

DiffTrain build for commit cb24396.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants