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 for Context as JSX #4618

Merged
merged 14 commits into from
Jan 14, 2025
Merged

Allow for Context as JSX #4618

merged 14 commits into from
Jan 14, 2025

Conversation

JoviDeCroock
Copy link
Member

@JoviDeCroock JoviDeCroock commented Dec 23, 2024

This PR adds another piece of React 19 functionality where we allow the result of createContext to act as a Provider.

Relates to #4613
Depends on preactjs/preact-devtools#493

@JoviDeCroock JoviDeCroock requested a review from Copilot December 23, 2024 08:29

Choose a reason for hiding this comment

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Copy link

github-actions bot commented Dec 23, 2024

📊 Tachometer Benchmark Results

Summary

duration

  • create10k: unsure 🔍 -0% - +1% (-0.32ms - +7.43ms)
    preact-local vs preact-main
  • filter-list: unsure 🔍 -1% - +5% (-0.10ms - +0.78ms)
    preact-local vs preact-main
  • hydrate1k: unsure 🔍 -2% - +1% (-1.09ms - +0.79ms)
    preact-local vs preact-main
  • many-updates: unsure 🔍 -3% - +2% (-0.53ms - +0.27ms)
    preact-local vs preact-main
  • replace1k: unsure 🔍 -2% - +1% (-1.48ms - +0.51ms)
    preact-local vs preact-main
  • text-update: unsure 🔍 -5% - +4% (-0.09ms - +0.09ms)
    preact-local vs preact-main
  • todo: unsure 🔍 -1% - +1% (-0.26ms - +0.19ms)
    preact-local vs preact-main
  • update10th1k: unsure 🔍 -5% - +2% (-1.62ms - +0.66ms)
    preact-local vs preact-main

usedJSHeapSize

  • create10k: unsure 🔍 -0% - -0% (-0.00ms - -0.00ms)
    preact-local vs preact-main
  • filter-list: unsure 🔍 -0% - +1% (-0.00ms - +0.02ms)
    preact-local vs preact-main
  • hydrate1k: unsure 🔍 -2% - +5% (-0.25ms - +0.48ms)
    preact-local vs preact-main
  • many-updates: faster ✔ 0% - 1% (0.00ms - 0.02ms)
    preact-local vs preact-main
  • replace1k: unsure 🔍 -0% - +0% (-0.01ms - +0.01ms)
    preact-local vs preact-main
  • text-update: unsure 🔍 -0% - -0% (-0.00ms - -0.00ms)
    preact-local vs preact-main
  • todo: unsure 🔍 -0% - +1% (-0.00ms - +0.02ms)
    preact-local vs preact-main
  • update10th1k: unsure 🔍 -0% - +0% (-0.01ms - +0.01ms)
    preact-local vs preact-main

Results

create10k

duration

VersionAvg timevs preact-localvs preact-main
preact-local947.61ms - 954.14ms-unsure 🔍
-0% - +1%
-0.32ms - +7.43ms
preact-main945.22ms - 949.41msunsure 🔍
-1% - +0%
-7.43ms - +0.32ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local19.20ms - 19.20ms-unsure 🔍
-0% - -0%
-0.00ms - -0.00ms
preact-main19.20ms - 19.21msunsure 🔍
+0% - +0%
+0.00ms - +0.00ms
-
filter-list

duration

VersionAvg timevs preact-localvs preact-main
preact-local16.50ms - 17.37ms-unsure 🔍
-1% - +5%
-0.10ms - +0.78ms
preact-main16.56ms - 16.63msunsure 🔍
-5% - +1%
-0.78ms - +0.10ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local1.52ms - 1.54ms-unsure 🔍
-0% - +1%
-0.00ms - +0.02ms
preact-main1.52ms - 1.52msunsure 🔍
-1% - +0%
-0.02ms - +0.00ms
-
hydrate1k

duration

VersionAvg timevs preact-localvs preact-main
preact-local66.22ms - 67.53ms-unsure 🔍
-2% - +1%
-1.09ms - +0.79ms
preact-main66.35ms - 67.70msunsure 🔍
-1% - +2%
-0.79ms - +1.09ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local10.21ms - 10.71ms-unsure 🔍
-2% - +5%
-0.25ms - +0.48ms
preact-main10.08ms - 10.61msunsure 🔍
-5% - +2%
-0.48ms - +0.25ms
-
many-updates

duration

VersionAvg timevs preact-localvs preact-main
preact-local16.69ms - 17.26ms-unsure 🔍
-3% - +2%
-0.53ms - +0.27ms
preact-main16.83ms - 17.39msunsure 🔍
-2% - +3%
-0.27ms - +0.53ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local3.75ms - 3.76ms-faster ✔
0% - 1%
0.00ms - 0.02ms
preact-main3.76ms - 3.78msslower ❌
0% - 1%
0.00ms - 0.02ms
-
replace1k

duration

VersionAvg timevs preact-localvs preact-main
preact-local72.48ms - 73.85ms-unsure 🔍
-2% - +1%
-1.48ms - +0.51ms
preact-main72.93ms - 74.37msunsure 🔍
-1% - +2%
-0.51ms - +1.48ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local2.96ms - 2.97ms-unsure 🔍
-0% - +0%
-0.01ms - +0.01ms
preact-main2.96ms - 2.97msunsure 🔍
-0% - +0%
-0.01ms - +0.01ms
-

run-warmup-0

VersionAvg timevs preact-localvs preact-main
preact-local31.27ms - 31.87ms-unsure 🔍
-2% - +1%
-0.57ms - +0.23ms
preact-main31.48ms - 32.01msunsure 🔍
-1% - +2%
-0.23ms - +0.57ms
-

run-warmup-1

VersionAvg timevs preact-localvs preact-main
preact-local36.59ms - 37.95ms-unsure 🔍
-3% - +3%
-1.02ms - +1.03ms
preact-main36.50ms - 38.03msunsure 🔍
-3% - +3%
-1.03ms - +1.02ms
-

run-warmup-2

VersionAvg timevs preact-localvs preact-main
preact-local27.41ms - 27.86ms-unsure 🔍
-0% - +2%
-0.09ms - +0.52ms
preact-main27.21ms - 27.63msunsure 🔍
-2% - +0%
-0.52ms - +0.09ms
-

run-warmup-3

VersionAvg timevs preact-localvs preact-main
preact-local28.59ms - 29.99ms-slower ❌
1% - 7%
0.20ms - 2.00ms
preact-main27.63ms - 28.76msfaster ✔
1% - 7%
0.20ms - 2.00ms
-

run-warmup-4

VersionAvg timevs preact-localvs preact-main
preact-local25.79ms - 27.23ms-unsure 🔍
-3% - +5%
-0.74ms - +1.31ms
preact-main25.50ms - 26.95msunsure 🔍
-5% - +3%
-1.31ms - +0.74ms
-

run-final

VersionAvg timevs preact-localvs preact-main
preact-local23.25ms - 23.76ms-unsure 🔍
-3% - +0%
-0.65ms - +0.10ms
preact-main23.50ms - 24.05msunsure 🔍
-0% - +3%
-0.10ms - +0.65ms
-
text-update
  • Browser: chrome-headless
  • Sample size: 100
  • Built by: CI #4273
  • Commit: 37bcc73

duration

VersionAvg timevs preact-localvs preact-main
preact-local1.92ms - 2.03ms-unsure 🔍
-5% - +4%
-0.09ms - +0.09ms
preact-main1.91ms - 2.05msunsure 🔍
-4% - +5%
-0.09ms - +0.09ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local1.11ms - 1.11ms-unsure 🔍
-0% - -0%
-0.00ms - -0.00ms
preact-main1.11ms - 1.11msunsure 🔍
+0% - +0%
+0.00ms - +0.00ms
-
todo

duration

VersionAvg timevs preact-localvs preact-main
preact-local33.84ms - 34.05ms-unsure 🔍
-1% - +1%
-0.26ms - +0.19ms
preact-main33.78ms - 34.18msunsure 🔍
-1% - +1%
-0.19ms - +0.26ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local1.22ms - 1.24ms-unsure 🔍
-0% - +1%
-0.00ms - +0.02ms
preact-main1.22ms - 1.22msunsure 🔍
-1% - +0%
-0.02ms - +0.00ms
-
update10th1k

duration

VersionAvg timevs preact-localvs preact-main
preact-local33.22ms - 34.59ms-unsure 🔍
-5% - +2%
-1.62ms - +0.66ms
preact-main33.47ms - 35.30msunsure 🔍
-2% - +5%
-0.66ms - +1.62ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local2.91ms - 2.92ms-unsure 🔍
-0% - +0%
-0.01ms - +0.01ms
preact-main2.91ms - 2.92msunsure 🔍
-0% - +0%
-0.01ms - +0.01ms
-

tachometer-reporter-action v2 for CI

Copy link

github-actions bot commented Dec 23, 2024

Size Change: +1 B (0%)

Total Size: 62.4 kB

Filename Size Change
dist/preact.js 4.7 kB +3 B (+0.06%)
dist/preact.min.js 4.72 kB +1 B (+0.02%)
dist/preact.min.module.js 4.72 kB -1 B (-0.02%)
dist/preact.min.umd.js 4.74 kB +3 B (+0.06%)
dist/preact.module.js 4.71 kB -5 B (-0.11%)
ℹ️ View Unchanged
Filename Size
compat/dist/compat.js 4.12 kB
compat/dist/compat.module.js 4.05 kB
compat/dist/compat.umd.js 4.19 kB
debug/dist/debug.js 3.83 kB
debug/dist/debug.module.js 3.83 kB
debug/dist/debug.umd.js 3.91 kB
devtools/dist/devtools.js 260 B
devtools/dist/devtools.module.js 274 B
devtools/dist/devtools.umd.js 346 B
dist/preact.umd.js 4.76 kB
hooks/dist/hooks.js 1.54 kB
hooks/dist/hooks.module.js 1.57 kB
hooks/dist/hooks.umd.js 1.61 kB
jsx-runtime/dist/jsxRuntime.js 978 B
jsx-runtime/dist/jsxRuntime.module.js 952 B
jsx-runtime/dist/jsxRuntime.umd.js 1.05 kB
test-utils/dist/testUtils.js 473 B
test-utils/dist/testUtils.module.js 477 B
test-utils/dist/testUtils.umd.js 555 B

compressed-size-action

@JoviDeCroock JoviDeCroock marked this pull request as ready for review December 29, 2024 10:42
@JoviDeCroock JoviDeCroock force-pushed the allow-for-context-as-jsx branch from bcb933b to 1006fd9 Compare December 29, 2024 10:44
@coveralls
Copy link

coveralls commented Dec 29, 2024

Coverage Status

coverage: 99.617% (-0.001%) from 99.618%
when pulling 37bcc73 on allow-for-context-as-jsx
into d16a34e on main.

src/create-element.js Outdated Show resolved Hide resolved
@JoviDeCroock JoviDeCroock requested a review from Copilot December 30, 2024 06:50

Choose a reason for hiding this comment

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

Copilot reviewed 6 out of 8 changed files in this pull request and generated no comments.

Files not reviewed (2)
  • compat/src/render.js: Evaluated as low risk
  • compat/test/browser/render.test.js: Evaluated as low risk
@JoviDeCroock JoviDeCroock force-pushed the allow-for-context-as-jsx branch from bdc5f94 to e471217 Compare December 30, 2024 06:51
@JoviDeCroock JoviDeCroock force-pushed the allow-for-context-as-jsx branch from e471217 to 4163c13 Compare December 30, 2024 06:52
@developit
Copy link
Member

developit commented Dec 31, 2024

I'm good with this, though I do wonder if we should just implement it in core.

import { enqueueRender } from './component';

export let i = 0;

export function createContext(defaultValue) {
  /** @type {import('./internal').FunctionComponent} */
  function Context(props) {
    if (!this.getChildContext) {
      /** @type {Set<import('./internal').Component> | null} */
      let subs = new Set();
      let ctx = {};
      ctx[Context._id] = this;

      this.getChildContext = () => ctx;

      this.componentWillUnmount = () => {
        subs = null;
      };

      this.shouldComponentUpdate = function (_props) {
        if (this.props.value !== _props.value) {
          subs.forEach((c) => {
            c._force = true;
            enqueueRender(c);
          });
        }
      };

      this.sub = (c) => {
        subs.add(c);
        let old = c.componentWillUnmount;
        c.componentWillUnmount = () => {
          if (subs) {
            subs.delete(c);
          }
          if (old) old.call(c);
        };
      };
    }

    return props.children;
  }

  Context._id = '__cC' + i++;
  Context._defaultValue = defaultValue;

  /** @type {import('./internal').FunctionComponent} */
  Context.Consumer = (props, contextValue) => {
    return props.children(contextValue);
  };

  // we could also get rid of _contextRef entirely
  Context.Provider = Context._contextRef = Context.Consumer.contextType = Context;

  return Context;
}

@marvinhagemeister
Copy link
Member

+1 for core if the snippet @developit provided works

@JoviDeCroock JoviDeCroock force-pushed the allow-for-context-as-jsx branch from b22f51d to d310b3a Compare December 31, 2024 09:13
@JoviDeCroock
Copy link
Member Author

@developit @marvinhagemeister added the suggestion

@JoviDeCroock JoviDeCroock force-pushed the allow-for-context-as-jsx branch from d310b3a to 75beb76 Compare December 31, 2024 10:20
mangle.json Outdated Show resolved Hide resolved
@JoviDeCroock JoviDeCroock merged commit b5eecc2 into main Jan 14, 2025
13 checks passed
@JoviDeCroock JoviDeCroock deleted the allow-for-context-as-jsx branch January 14, 2025 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants