-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Change assert to warn for unneeded restriction around creating sub-applications #20915
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
base: main
Are you sure you want to change the base?
Change assert to warn for unneeded restriction around creating sub-applications #20915
Conversation
Estimated Asset SizesDiff --- main/out.txt 2025-06-23 18:17:38.000000000 +0000
+++ pr/./pr-16034637610/out.txt 2025-07-02 20:03:10.000000000 +0000
@@ -1,41 +1,41 @@
╔═══════╤═══════════╤═══════════╗
║ │ Min │ Gzip ║
╟───────┼───────────┼───────────╢
-║ Total │ 408.84 KB │ 228.88 KB ║
+║ Total │ 408.85 KB │ 228.86 KB ║
╚═══════╧═══════════╧═══════════╝
-╔══════════════════════╤═══════════╤══════════╗
-║ @ember/* │ Min │ Gzip ║
-╟──────────────────────┼───────────┼──────────╢
-║ Total │ 239.23 KB │ 147.2 KB ║
-╟──────────────────────┼───────────┼──────────╢
-║ -internals │ 35.44 KB │ 25.49 KB ║
-║ application │ 12.83 KB │ 7.62 KB ║
-║ array │ 12.66 KB │ 7.32 KB ║
-║ canary-features │ 304 B │ 419 B ║
-║ component │ 1.07 KB │ 1004 B ║
-║ controller │ 1.8 KB │ 1.36 KB ║
-║ debug │ 11.4 KB │ 7.92 KB ║
-║ deprecated-features │ 31 B │ 77 B ║
-║ destroyable │ 561 B │ 383 B ║
-║ enumerable │ 259 B │ 387 B ║
-║ helper │ 823 B │ 570 B ║
-║ instrumentation │ 2.43 KB │ 1.78 KB ║
-║ modifier │ 669 B │ 614 B ║
-║ object │ 33.78 KB │ 20.79 KB ║
-║ owner │ 159 B │ 178 B ║
-║ renderer │ 385 B │ 327 B ║
-║ routing │ 58.05 KB │ 33.43 KB ║
-║ runloop │ 2.2 KB │ 1.33 KB ║
-║ service │ 859 B │ 741 B ║
-║ template │ 430 B │ 390 B ║
-║ template-compilation │ 429 B │ 366 B ║
-║ template-compiler │ 57.81 KB │ 30.44 KB ║
-║ template-factory │ 94 B │ 160 B ║
-║ test │ 923 B │ 627 B ║
-║ utils │ 3.93 KB │ 3.5 KB ║
-║ version │ 55 B │ 131 B ║
-╚══════════════════════╧═══════════╧══════════╝
+╔══════════════════════╤═══════════╤═══════════╗
+║ @ember/* │ Min │ Gzip ║
+╟──────────────────────┼───────────┼───────────╢
+║ Total │ 239.24 KB │ 147.18 KB ║
+╟──────────────────────┼───────────┼───────────╢
+║ -internals │ 35.45 KB │ 25.47 KB ║
+║ application │ 12.83 KB │ 7.62 KB ║
+║ array │ 12.66 KB │ 7.32 KB ║
+║ canary-features │ 304 B │ 419 B ║
+║ component │ 1.07 KB │ 1004 B ║
+║ controller │ 1.8 KB │ 1.36 KB ║
+║ debug │ 11.4 KB │ 7.92 KB ║
+║ deprecated-features │ 31 B │ 77 B ║
+║ destroyable │ 561 B │ 383 B ║
+║ enumerable │ 259 B │ 387 B ║
+║ helper │ 823 B │ 570 B ║
+║ instrumentation │ 2.43 KB │ 1.78 KB ║
+║ modifier │ 669 B │ 614 B ║
+║ object │ 33.78 KB │ 20.79 KB ║
+║ owner │ 159 B │ 178 B ║
+║ renderer │ 385 B │ 327 B ║
+║ routing │ 58.05 KB │ 33.43 KB ║
+║ runloop │ 2.2 KB │ 1.33 KB ║
+║ service │ 859 B │ 741 B ║
+║ template │ 430 B │ 390 B ║
+║ template-compilation │ 429 B │ 366 B ║
+║ template-compiler │ 57.81 KB │ 30.44 KB ║
+║ template-factory │ 94 B │ 160 B ║
+║ test │ 923 B │ 627 B ║
+║ utils │ 3.93 KB │ 3.5 KB ║
+║ version │ 55 B │ 131 B ║
+╚══════════════════════╧═══════════╧═══════════╝
╔═════════════════╤═══════════╤══════════╗
║ @glimmer/* │ Min │ Gzip ║ Details
|
@ef4 Are you okay with this as a way to unblock @NullVoxPopuli without needing to fully deprecate and remove EventDispatcher (yet)? |
No, I don't think this is a good idea. Either we support this case or we don't. If we do, then we should just drop the assertion entirely as in #20910. If we don't, then the assertion should stay. Until then @NullVoxPopuli can opt into "I know this is unsupported" mode by continuing to use pnpm patch. Also it makes no sense to only change this assertion without also changing the assertions around it. If "descendant of existing ember app" is OK then so is "ancestor of existing ember app", for example. |
makes sense -- I'd prefer to remove the assertions altogether as in https://github.com/emberjs/ember.js/pull/20910/files but I don't yet know where this falls down. I know this is all in event_dispatcher code, but what uses the event_dispatcher? How would someone know if they've created a broken situation? |
Just from searching, it looks like @ember/component uses EventDispatcher for |
Alternative to: #20910
PR description from there:
A path to unblocking:
While working new repl infra for https://limber.glimdown.com, I want to take a more "islands" approach, like https://astro.build -- it enables a ton of power -- however, there is an assertion that prevents using nested applications -- which kinda feels silly since we allow it via "engines".
Like with how we use modifiers and 3rd party libraries that manage DOM, whenever we use an element that has some external control of its descendents, we must make the same assumptions and allowances --- generally this is safe, as once we let 3rd party scripts manage their subtree, ember doesn't mess around in there (because when someone would instruct ember to mess around in someone else's subtree, that's when you run in to problems -- but this is easy enough to avoid).
In my use case, I have:
so this component is mounting a whole new app, and nothing is is going to mess with the subtree.
This works via using the new
template()
API for the runtime compiler: