Simplify SimpleDOM casting (allow for proper production mode stripping)
#1162
Conversation
The new `cast` function added for the recent TS upgrade has a couple of issues: 1. It creates wrapping objects which modify the values that are wrapped/casted. Removing `cast()`, as the production build currently does, breaks because of this. 2. These wrapping objects conflate casting _to_ SimpleDOM and casting _away_ from it, into browser APIs. The new system makes `cast` into two pure validation utility functions, which either downlevels to SimpleDOM (without any checks, as SimpleDOM is a subset of the real DOM), or uplevels to DOM with appropriate checks (e.g. are we actually in the browser? Is the node a DOM node? Is it the expected type of DOM node?).
pzuraq
force-pushed
the
bugfix/simply-simple-cast
branch
from
95d2a4c
to
fdfb0c2
Compare
Where/when were these issues identified?
Does this indicate a lack of testing? Specifically, it would seem that we probably can't be testing the production builds (since this issue would have likely caused those tests to fail), right? |
| function isBrowserNode(node: Node | SimpleNode): node is Node { | ||
| return typeof document !== undefined && node.ownerDocument === document; | ||
| } | ||
| export function checkNode<S extends SugaryNodeCheck>( |
There was a problem hiding this comment.
Just to be clear, this is a DEBUG only utility (or actually, it may be a LOCAL_DEBUG?) right?
rwjblue
changed the title
SimpleDOM casting (allow for proper production mode stripping)
|
Hmm. I don't fully grok why this would impact the performance check (~2% / 21ms slower). 🤔
|
| @@ -30,13 +30,13 @@ class OnModifierManager implements ModifierManager<OnModifierState, null> { | |||
| install(state: OnModifierState) { | |||
| const name = valueForRef(state.nameRef); | |||
| const listener = valueForRef(state.listenerRef); | |||
| cast(state.element, 'ELEMENT').addEventListener(name, listener); | |||
| castToBrowser(state.element, 'ELEMENT').addEventListener(name, listener); | |||
There was a problem hiding this comment.
Is this removed for @glimmer/benchmark-env performance test also?
There was a problem hiding this comment.
Ah, it may not be, we can check
|
Production builds are not tested, no. It’s a gap we should probably prioritize filling |
👍 totally agree, created #1165 to track that |
pzuraq
force-pushed
the
bugfix/simply-simple-cast
branch
from
80fd642
to
fdfb0c2
Compare
|
It looks like there was some code that was not running at all in production builds due to the casting failure, specfically: Tested by disabling these on this branch, and it seemed to move the needle back to mostly no stat-sig change, so I think that may have been the cause for the perf difference. This was clearly a bug, so I think it makes sense to merge and if we need to fixup perf in the future, find somewhere else to fix things. |
The new
castfunction added for the recent TS upgrade has a couple ofissues:
wrapped/casted. Removing
cast(), as the production build currentlydoes, breaks because of this.
away from it, into browser APIs.
The new system makes
castinto two pure validation utility functions,which either downlevels to SimpleDOM (without any checks, as SimpleDOM
is a subset of the real DOM), or uplevels to DOM with appropriate checks
(e.g. are we actually in the browser? Is the node a DOM node? Is it the
expected type of DOM node?).