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

[joke] its not fine #28524

Closed
wants to merge 1 commit into from
Closed

Conversation

rickhanlonii
Copy link
Member

cheeky idea for blocking this import

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 8, 2024
@react-sizebot
Copy link

react-sizebot commented Mar 8, 2024

Comparing: 64f354c...294322c

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 = 177.10 kB 177.10 kB = 55.20 kB 55.20 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 177.64 kB 177.64 kB = 55.53 kB 55.53 kB
facebook-www/ReactDOM-prod.classic.js = 594.34 kB 594.34 kB = 104.96 kB 104.96 kB
facebook-www/ReactDOM-prod.modern.js = 577.60 kB 577.60 kB = 102.02 kB 102.02 kB
test_utils/ReactAllWarnings.js Deleted 66.60 kB 0.00 kB Deleted 16.28 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
oss-stable-semver/react/cjs/react.development.js +0.30% 98.43 kB 98.72 kB +0.44% 26.52 kB 26.63 kB
oss-stable/react/cjs/react.development.js +0.30% 98.46 kB 98.75 kB +0.44% 26.55 kB 26.66 kB
facebook-react-native/react/cjs/React-dev.js +0.29% 122.04 kB 122.39 kB +0.46% 29.29 kB 29.42 kB
oss-experimental/react/cjs/react.development.js +0.29% 101.07 kB 101.37 kB +0.42% 27.38 kB 27.50 kB
facebook-www/React-dev.modern.js +0.29% 124.16 kB 124.52 kB +0.41% 29.56 kB 29.68 kB
facebook-www/React-dev.classic.js +0.28% 125.75 kB 126.10 kB +0.37% 29.94 kB 30.05 kB
oss-stable-semver/react/umd/react.development.js +0.26% 121.02 kB 121.33 kB +0.39% 31.09 kB 31.21 kB
oss-stable/react/umd/react.development.js +0.26% 121.04 kB 121.36 kB +0.39% 31.11 kB 31.23 kB
oss-experimental/react/umd/react.development.js +0.25% 123.76 kB 124.07 kB +0.37% 31.99 kB 32.11 kB
test_utils/ReactAllWarnings.js Deleted 66.60 kB 0.00 kB Deleted 16.28 kB 0.00 kB

Generated by 🚫 dangerJS against 294322c

@markerikson
Copy link
Contributor

What would be needed to enable cross-renderer contexts instead?

@superhooman
Copy link

What if it’s installed globally?

@rickhanlonii rickhanlonii changed the title its not fine [joke] its not fine Mar 12, 2024
@rickhanlonii
Copy link
Member Author

@markerikson this is a cheeky joke. Obviously we wouldn't land this.

The issue to track that feature request is still #13332. It's unclear how to implement support for that feature, but we've been talking to @drcmda about options for third-party renderers. When we have more info, we'll update the issue.

@sebmarkbage
Copy link
Collaborator

Spent some time investigating. Regardless the versions would have to be in perfect lock-step but that turns out to be a requirement anyway so that's fine.

The proper solution that behaves like a Portal should forward the value during a concurrent render into the child. Meaning it should actually be part of the same render phase. That's not just an issue with Context but Props too. Ideally it shouldn't have to commit the outer root before forwarding the inner values.

Additionally, it's not just Context that is contextual. There's internal contexts and for example if you Suspend in the child renderer outside a Suspense boundary, it should affect the parent render. E.g. a startTransition in the parent should stall or .

That's the proper implementation. It's very difficult, and may require compromising on performance for everyone, but it's not impossible. Realistically, that amount of work, I don't see that getting prioritized any time soon. There's just way more pressing issues.

The smaller version of converting an outer render into a sync secondary render after commit, with no Suspense or Transition integration etc. is easier. That might be "good enough". However, there's the question of whether encouraging thinking of these is a single root given that new features won't work seamlessly anyway. So it might've been "good enough" before but not "good enough" in the future.

There's also a whole other approach of creating fake DOM nodes using the DOM renderer.

@drcmda
Copy link

drcmda commented Mar 13, 2024

Spent some time investigating. Regardless the versions would have to be in perfect lock-step but that turns out to be a requirement anyway so that's fine.

The proper solution that behaves like a Portal should forward the value during a concurrent render into the child. Meaning it should actually be part of the same render phase. That's not just an issue with Context but Props too. Ideally it shouldn't have to commit the outer root before forwarding the inner values.

Additionally, it's not just Context that is contextual. There's internal contexts and for example if you Suspend in the child renderer outside a Suspense boundary, it should affect the parent render. E.g. a startTransition in the parent should stall or .

That's the proper implementation. It's very difficult, and may require compromising on performance for everyone, but it's not impossible. Realistically, that amount of work, I don't see that getting prioritized any time soon. There's just way more pressing issues.

The smaller version of converting an outer render into a sync secondary render after commit, with no Suspense or Transition integration etc. is easier. That might be "good enough". However, there's the question of whether encouraging thinking of these is a single root given that new features won't work seamlessly anyway. So it might've been "good enough" before but not "good enough" in the future.

There's also a whole other approach of creating fake DOM nodes using the DOM renderer.

hello @sebmarkbage we hacked the context via its-fine by injecting it https://github.com/pmndrs/its-fine/blob/main/src/index.tsx#L217-L234 a renderer must wrap its contents into a bridge. this of course is tapping into fiber internals.

we hacked suspense as well as error boundaries with a throw. now the renderer can suspend the dom while loading assets, or be caught in the dom if it fails. we would be very happy if there are official means.

export function Block({ set }) {
  useIsomorphicLayoutEffect(() => {
    set(new Promise(() => null))
    return () => set(false)
  }, [set])
  return null
}

function Canvas(...) {
  ...
  const [block, setBlock] = React.useState(false)
  const [error, setError] = React.useState(false)

  // Suspend this component if block is a promise (2nd run)
  if (block) throw block
  // Throw exception outwards if anything within canvas throws
  if (error) throw error

  ...

      root.current.render(
        <Bridge>
          <ErrorBoundary set={setError}>
            <React.Suspense fallback={<Block set={setBlock} />}>{children}</React.Suspense>
          </ErrorBoundary>
        </Bridge>,
      )

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

7 participants