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

Allow passing client exports through async code #26666

Closed
wants to merge 1 commit into from

Conversation

sophiebits
Copy link
Collaborator

No description provided.

@react-sizebot
Copy link

Comparing: da6c23a...91b3cc6

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 = 164.42 kB 164.42 kB = 51.69 kB 51.69 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 166.86 kB 166.86 kB = 52.34 kB 52.34 kB
facebook-www/ReactDOM-prod.classic.js = 564.45 kB 564.45 kB = 99.40 kB 99.40 kB
facebook-www/ReactDOM-prod.modern.js = 548.24 kB 548.24 kB = 96.71 kB 96.71 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-node-register.js +0.45% 4.19 kB 4.21 kB +0.33% 1.50 kB 1.51 kB
oss-stable-semver/react-server-dom-webpack/cjs/react-server-dom-webpack-node-register.js +0.45% 4.19 kB 4.21 kB +0.33% 1.50 kB 1.51 kB
oss-stable/react-server-dom-webpack/cjs/react-server-dom-webpack-node-register.js +0.45% 4.19 kB 4.21 kB +0.33% 1.50 kB 1.51 kB

Generated by 🚫 dangerJS against 91b3cc6

@facebook-github-bot facebook-github-bot added the React Core Team Opened by a member of the React Core Team label Apr 19, 2023
Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

This is a little tricky because Promise.resolve(...) is correct since it yields another Promise but then awaiting it is not correct.

Imagine the client export is Promise<string>, if you await it before passing it along it's supposed to unwrap it.

<ClientComponent text={await ClientModule.asyncText} />

Type checking this would pass because text would accept string and await asyncText unwraps the promise before passing it. The client reference is supposed to be a reference that unwraps it on the client before passing it along which this wouldn't do. So it's a runtime error.

We only really special case unwrapping shallowly for the module object since it's used to polyfill ESM.

@sophiebits
Copy link
Collaborator Author

Well what I ran into is

import { Client, somePojo } from './client.js';

async function getThing() {
  if (cond) {
    return await fetch(...);
  } else {
    return somePojo;
  }
}

async function App() {
  const thing = await getThing();
  return <Client data={thing} />;
}

which felt like it should work. I could make a second async flag on the client reference that awaits the named export. Does that sound sensible?

@sebmarkbage
Copy link
Collaborator

Not really, we have to put a limit to how much we encode in this otherwise we might as well have infinite dotting into things but the further you go, the more confusing it gets and the more we have to encode. The async module thing is only applicable to CommonJS and not ESM. So it's already not a general purpose mechanism.

The semantics are really that you can just pass an identifier from the import to the client, without touching it.

Because this is syntactic, it doesn't feel like you are but because of the weird Promise resolution it does.

Maybe we need a better error message - but I feel like the error is still appropriate.

A possible workaround is wrapping it in an object like return {data: somePojo} ... (await getThing()).data.

@sophiebits
Copy link
Collaborator Author

Sad. Thanks.

@sophiebits sophiebits closed this Apr 19, 2023
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

4 participants