Upgrade SimpleDOM and use DOM types more consistently #866
Conversation
| export { | ||
| ElementNamespace, | ||
| AttrNamespace, | ||
| SimpleNode as Node, |
There was a problem hiding this comment.
shadowing ambient interfaces can be a bit trollish, but I'm ok you are just trying to minimize the scope of the change here
There was a problem hiding this comment.
I agree that shadowing is trollish. We avoid maximum troll by not making it easy to import these types directly, and re-export the entire module under the Simple namespace from the package root. In practice, I don't think we ever refer to bare Node, etc. in the codebase, and always as Simple.Node.
| host.appendChild(this.element); | ||
| host.appendChild(clientRemote); | ||
| clientRemote.innerHTML = serializedRemote; | ||
| this.element = host.firstChild as HTMLElement; | ||
| (clientRemote as Element).innerHTML = serializedRemote; |
There was a problem hiding this comment.
can we hide this in a TestHost interface? that provides this op and whether it is supported by the underlying DOM? it would be nice to get to the point we can run the whole suite in SSR vs not for most tests.
There was a problem hiding this comment.
I tried untangling this one but it was quite hairy and I was afraid of making substantive changes in a PR this big. Will follow up with you offline because I agree we should clean this up.
| @@ -30,7 +30,7 @@ function commonSetup() { | |||
| function render(template: Template, context = {}) { | |||
| self = new UpdatableReference(context); | |||
| env.begin(); | |||
| let cursor = { element: root, nextSibling: null }; | |||
| let cursor = { element: root, nextSibling: null } as Cursor; | |||
There was a problem hiding this comment.
is Cursor an interface or is it a class? let cursor: Cursor = { element: root, nextSibling: null }
There was a problem hiding this comment.
It's actually a class for non-obvious reasons, but used like an interface almost everywhere. The cast is necessary because root is an HTMLElement and needs to be explicitly cast to the expected Simple.Element type.
tomdale
force-pushed
the
tomdale/upgrade-simple-dom-1-4
branch
from
0c66e04
to
9411464
Compare
This PR makes several changes related to how we handle types for DOM objects in Glimmer VM. Specifically, this PR:
Perhaps the most important impact of this change is that downstream consumers, such as Glimmer.js and Ember.js, will be able remove lots of the nasty type coercion (e.g.
document.body as any as Element) that is required today when you pass real DOM elements into Glimmer VM APIs.SimpleDOM Upgrade
This PR replaces an older version of
simple-domwith the newer packages@simple-dom/document,@simple-dom/serializer,@simple-dom/void-map, and@simple-dom/interface, where appropriate. It also updates the build script to use the prebuilt AMD versions of these packages, rather than compiling SimpleDOM from scratch.Within the VM, DOM types are imported as the
Simplenamespace from@glimmer/interfaces, which itself simply re-exports the types defined in SimpleDOM. I took this approach, rather than importing types from@simple-dom/interfacedirectly in runtime code, to make it easier to decouple our types from SimpleDOM in the future if need be.The one exception to this rule is cases where runtime code uses
const enumtypes, likeNamespaceandInsertPosition. This is due to an apparent type erasure bug in the TypeScript compiler, where importedconst enums that go through a re-export are not properly removed in emitted JavaScript, causing the Rollup build to fail (since it tries to import type definitions that no longer exist). This may be fixed in newer versions of TypeScript and we can ideally change this once the bug is fixed.Ubiquitous SimpleDOM Types
Previously, types for DOM objects were used inconsistently throughout the codebase. Often
Simple.*,simple-domor native DOM types were used interchangeably. Unifying theSimple.*andsimple-domtypes helps, but leaves the inconsistency with the native DOM types.In this PR, I've adopted the rule that all types should be
Simple.*types unless the function in question utilizes APIs not provided by SimpleDOM. In those cases, the function is responsible for performing any necessary type coercion. This helps make code that is not SSR safe more explicit.In the case of tests, which tend to rely on non-simple DOM APIs much more (e.g. setting
innerHTML), I've added new APIs to the test infrastructure that provide natively-typed versions of DOM objects along with assertions that verify they are running in a real browser environment. My hope is that this makes tracking down browser-dependent tests easier, and in the future we can ensure that tests aren't inadvertently coupled to the browser if they would be helpful to run in SSR mode as well.