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

Inject react-dom in the shared scope #1320

Merged
merged 1 commit into from
Apr 21, 2023

Conversation

martinRenou
Copy link
Member

References

Similar to voila-dashboards/voici#67

This fixes support for ipyflex

@martinRenou martinRenou added the bug Something isn't working label Apr 21, 2023
@github-actions
Copy link
Contributor

Binder 👈 Launch a Binder on branch martinRenou/voila/inject_react_dom

@github-actions
Copy link
Contributor

Benchmark report

The execution time (in milliseconds) are grouped by test file, test type and browser.
For each case, the following values are computed: min <- [1st quartile - median - 3rd quartile] -> max.

Results table
Test file voila-tree-classic.ipynb voila-tree-light.ipynb voila-tree-dark.ipynb voila-tree-miami.ipynb basics.ipynb bqplot.ipynb dashboard.ipynb gridspecLayout.ipynb interactive.ipynb ipympl.ipynb mimerenderers.ipynb bokeh.ipynb multiple_widgets.ipynb query-strings.ipynb reveal.ipynb
Render
chromium
actual 128 <- [142 - 169 - 221] -> 347 90 <- [93 - 96 - 103] -> 131 70 <- [74 - 81 - 92] -> 117 83 <- [87 - 95 - 106] -> 135 3046 <- [3147 - 3245 - 3513] -> 4834 2542 <- [2583 - 2704 - 2851] -> 3096 2656 <- [2744 - 2765 - 2791] -> 3065 2902 <- [2904 - 2935 - 2988] -> 3175 2606 <- [2673 - 2694 - 2741] -> 3259 3550 <- [3626 - 3771 - 3897] -> 4617 7986 <- [8034 - 8104 - 8105] -> 8680 4300 <- [4379 - 4518 - 4880] -> 5747 4332 <- [4363 - 4407 - 4498] -> 4663 1869 <- [1910 - 1913 - 1953] -> 2123 3880 <- [4098 - 4541 - 5206] -> 6912
expected 3379 <- [3442 - 3517 - 3701] -> 3876 2976 <- [3227 - 3321 - 3421] -> 3604 3608 <- [3623 - 3709 - 3793] -> 3825 4453 <- [4453 - 4523 - 4661] -> 4748 2559 <- [2655 - 2656 - 2660] -> 2674 3982 <- [4079 - 4213 - 4356] -> 4743 12183 <- [18509 - 19553 - 20811] -> 21515 15319 <- [15660 - 15796 - 15912] -> 16056 1517 <- [1920 - 1997 - 2103] -> 2113

❗ Test metadata have changed
--- /dev/fd/63	2023-04-21 09:30:52.781476210 +0000
+++ /dev/fd/62	2023-04-21 09:30:52.781476210 +0000
@@ -4,51 +4,49 @@
     "BENCHMARK_REFERENCE": "actual"
   },
   "browsers": {
-    "chromium": "112.0.5615.29"
+    "chromium": "94.0.4595.0"
   },
   "systemInformation": {
     "cpu": {
-      "brand": "Xeon® Platinum 8171M",
+      "brand": "Xeon® E5-2673 v3",
       "cache": {
         "l1d": 65536,
         "l1i": 65536,
-        "l2": 2097152,
-        "l3": 36700160
+        "l2": 524288,
+        "l3": 31457280
       },
       "cores": 2,
-      "efficiencyCores": 0,
       "family": "6",
-      "flags": "fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush mmx fxsr sse sse2 ss ht syscall nx pdpe1gb rdtscp lm constant_tsc rep_good nopl xtopology cpuid pni pclmulqdq ssse3 fma cx16 pcid sse4_1 sse4_2 movbe popcnt aes xsave avx f16c rdrand hypervisor lahf_lm abm 3dnowprefetch invpcid_single pti fsgsbase bmi1 hle avx2 smep bmi2 erms invpcid rtm mpx avx512f avx512dq rdseed adx smap clflushopt avx512cd avx512bw avx512vl xsaveopt xsavec xsaves md_clear",
+      "flags": "fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush mmx fxsr sse sse2 ss ht syscall nx pdpe1gb rdtscp lm constant_tsc rep_good nopl xtopology cpuid pni pclmulqdq ssse3 fma cx16 pcid sse4_1 sse4_2 movbe popcnt aes xsave avx f16c rdrand hypervisor lahf_lm abm invpcid_single pti fsgsbase bmi1 avx2 smep bmi2 erms invpcid xsaveopt md_clear",
       "governor": "",
-      "manufacturer": "Intel",
-      "model": "85",
-      "performanceCores": 2,
+      "manufacturer": "Intel®",
+      "model": "63",
       "physicalCores": 2,
       "processors": 1,
       "revision": "",
       "socket": "",
-      "speed": 2.6,
+      "speed": 2.4,
       "speedMax": null,
       "speedMin": null,
-      "stepping": "4",
-      "vendor": "Intel",
+      "stepping": "2",
+      "vendor": "GenuineIntel",
       "virtualization": false,
       "voltage": ""
     },
     "mem": {
-      "total": 7281270784
+      "total": 7291699200
     },
     "osInfo": {
       "arch": "x64",
       "build": "",
-      "codename": "Jammy Jellyfish",
+      "codename": "Focal Fossa",
       "codepage": "UTF-8",
       "distro": "Ubuntu",
-      "kernel": "5.15.0-1035-azure",
+      "kernel": "5.8.0-1040-azure",
       "logofile": "ubuntu",
       "platform": "linux",
-      "release": "22.04.2 LTS",
-      "serial": "a6219d4d787c4becbd42a2cf1ee91d94",
+      "release": "20.04.3 LTS",
+      "serial": "cfc067bfcb844f35865e279a1b0e66c5",
       "servicepack": "",
       "uefi": false
     }

@trungleduc
Copy link
Member

Thanks @martinRenou

@trungleduc trungleduc merged commit c49cfc8 into voila-dashboards:main Apr 21, 2023
@martinRenou martinRenou deleted the inject_react_dom branch April 21, 2023 09:47
@maartenbreddels
Copy link
Member

Why is this needed? What was the issue with ipyflex?
I don't think a widget library should depends on the environment providing react and react-dom. If a widget library wants to use a specific react version, jupyter-widgets/widget-ts-cookiecutter#130 (comment) should do that.
This PR might still be useful btw, but I've been fighting the battle recently. Maybe this information helps.

@martinRenou
Copy link
Member Author

That's due to the whole "shared package" logic in the webpack federated thingy https://github.com/trungleduc/ipyflex/blob/master/package.json#L99.

I think react needs to be specified as shared, we don't want all packages to provide their own react in the page, do we?
Also, does it actually work to have multiple react in the page? Wouldn't it break entirely?

@maartenbreddels
Copy link
Member

Multiple react version on the same page is no problem at all. And react is quite small:

The problem actually occurs when you get a wrong version of react and react-dom. Maybe a better strategy is strictVersion =true ( https://jupyterlab.readthedocs.io/en/stable/extension/extension_dev.html#deduplication-of-dependencies ) since I believe the default of that is false (the documentation doesn't say that, but otherwise I wouldn't see the issue).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants