feat(core): Add DOM & EventTarget #10223
Conversation
Nx Cloud ReportAttention: This version of the Nx Cloud GitHub bot will cease to function on July 1st, 2023. An organization admin can update your integration here. CI is running for commit 84b648d. 📂 Click to track the progress, see the status, the terminal output, and the build insights. Sent with 💌 from NxCloud. |
|
All tests pass but I keep seeing this crash: It is related to the Observable changes. Something very specific probably? If i add a couple of checks, then everything is ok. Basically |
|
Hey @NathanWalker most stuff related to DOM has been implemented in this PR without any breaking changes. This is a lot of work. I am wondering If I can get a Green Light to continue working on it to make it complete and probably add one or two flavors as an example. |
|
I am personally worried about performances hit of this PR. Especially when the change is not really needed in the sense it does not add more to N. I understand why you want that for flavors. But I am not sure all that should be part of N if it has perf consequences |
This PR is not about performance but conformance to "something" especially the DOM. Performance can be further optimized.
I am surpised to hear this after having so many debates over DOM. It solves the flavor integration problem. There has been a lot of debate in the community around adding a DOM and especially around dominative by @ClassicOldSong not a viable solution because a lot of people say As far as performance is concerned, It's fast because it's based on dominative by @ClassicOldSong. Event emitting is a little slower than current core if you use This allows us to slowly make Finally this might in future allow us to run NativeScript apps on web & mobile like react native and flutter do. It's something that is very high in demand from most devs and NativeScript lacks. What are your thoughts on this @NathanWalker |
|
I'm disappointed to see "performance" being an excuse for holding things back. It's already being tested multiple times that DOMiNATIVE isn't the main cause for slowdowns and it even provides more correctness than some existing flavors. Even though there're slight overhead with this dom implementation, there're also overheads with other implementations, and I've said many times in previous discussions about maintenance cost - one correctly implemented dom can benefit all other frameworks, which once again proved itself with the svelte comparison. What we can gain from this PR is much more than what we might lost. Time doesn't wait, there's no much time left to compete with RN and Flutter. |
Benchmarks:Without DOM:With DOM:As I mentioned before, using View creation calls seem a bit slow at first, but if you look at Other than that, core with DOM goes head to head with core without DOM. Theoretically all web frameworks may perform at near-core speed after looking at these benches |
|
@ammarahm-ed thanks for those benchmarks. Now about the result and your comment.
We want DOM compliance i can understand even if dont want/need it. But please remember that the first users of N are not the devs but the clients who accept to have their app implemented in N. Those do not care about DOM compliance, what they only care about is that their N apps are fast, responsive (as fast or faster than other platforms) and cheap. |
|
@farfromrefug on real device. Benchmarks compare results with same device running old and new core so results are directly comparable although i do have a low end device i will run benches with. |
On real device
Both DOM/Without Dom use the same code..so although it says new View() it's actually a flexbox layout same as without dom.
Notify takes some perf hit obviously but moving to dispatchEvent is the solution which is probably the simplest one. And it won't be a breaking change if you move a plugin to dispatchEvent on the version of {N} with DOM added. Although yes, the plugin won't work on older versions of {N} which is probably something that will have ultimately deal with at some point. Just upgrading your {N} version will fix the issue. But even then 600K ops is huge and i think it won't affect performance in a real world app at all. Especially that what truly makes core slower is when data comes from native or goes to native. The event itself is pretty fast. Ultimately for something better, you have to deal with breaking changes at some point. The positive thing is that everything works the same now with dom added. And any breaking changes will affect only plugins for older versions of {N}. Upgrading {n} would be the easiest fix if you have plugin that's critical for performance. Although i am sure most plugins will work fine with notify. I guess we can migrate some plugins to use dispatchEvent. We can obviously make this opt-in, i mean all i need to do check if it's a dom node or not and skip firing dispatchEvent and instead fire a simple event as is, which will remove the perf hit completely but it would be better to use this slowdown as a bait to make all framework implementations conform to the same feature set and use the DOM in core.
Every framework implements a DOM of it's own anyway, this just enables us to do more for all frameworks in the same way. I don't see us losing performance since i am comparing with bare bones core that's probably faster than all web framework implementations you can find for {N}. It will actually perform and conform better. {N} clients will be happy to see better plugins and support eventually. It's also very discouraging to know that devs are a secondary priority for {N} if this is true. Because I think {N} is for devs unless I am wrong about it and you only want to build apps using {N} alone and not make it nice for everyone else. Otherwise one shouldn't compare {N} with RN or flutter since those frameworks "are" for the devs, then anyone else. And to be honest if devs are satisfied, clients can be delt with too.
We are not adding DOM compliance (maybe we are to some degree), we are just putting a DOM in core so all frameworks can work and function the same way internally and we can provide better support across frameworks. It also makes supporting and maintaining new frameworks easy and sustainable in long term. Today, i think react native & NativeScript both perform almost the same. {N} is not slow I don't think anyone will choose RN because {N} is slower, it would be more about ecosystem and the whole politics that goes around OSS. Dominative was already very fast with all the logic it added but now i think it will be faster in core. |
|
@farfromrefug After adding a small optimization, if you don't use DOM, notify has same performance as DOMLESS |
|
@ammarahm-ed that is an amazing news! |
|
@ammarahm-ed It might be a good idea to add |
Hmm 🤔 unless we have multiple windows in the app, we might do fine with a single window object in global being referenced as |
There was a problem hiding this comment.
As always, thanks for keeping the ball rolling on DOM. Just putting my thoughts together before the coming TSC meeting.
As a summary: this PR forks happy-dom and undom-ng to implement DOM, mostly separately, but with some significant changes to Core:
- The inheritance chain now goes:
Viewbase > HTMLElement > Element > ParentNode > Node > Observable > EventTarget. Much of these appear to be derived from happy-dom so I'm unclear which parts are from undom-ng (code comments pointing at the original files would help). Observablemaintains backwards-compatibility by changing the types ofaddEventListenerandremoveEventListenerto accept an arbitrary-length array of unknown args and making a best effort to guess whether the third argument isthisArgor EventListenerOptions.- A new class named
Eventis introduced to allow a bubbling/capturing/cancelling of DOM Events through a series of EventTargets, in place of the existing system where we just call a callback on a single notifier. The weird global event listeners are also still supported. - Gestures seem to be untouched (I'm not sure whether this is an oversight).
While I do support implementing DOM for NativeScript in one way or another, I'm afraid I'm not in favour of this exact approach.
I think writing a DOM implementation is all for nothing if the DOM types don't derive exactly from libdom.d.ts. Otherwise using DOM libraries will never be a seamless experience because the happy-dom types already do not match 1:1 with libdom.d.ts and even if they did, it's a moving target which updates with each version of TypeScript.
happy-dom and undom-ng are fine as a basis, but only for the implementation logic—I think each class provided by them should conform to the corresponding class/interface from libdom.d.ts.
Also I am concerned that this is a huge surface area of new code to add to Core. I think the easiest path to acceptance by other maintainers would be to add just the minimum of what would be needed to allow Core's events implementation to be customised from the outside. Not to say that it would be easy to design.
Even if we had a lot of support for a full refactor, I think the scope of this change is far too big to be reviewed properly in a single PR. I think this would be an easier conversation if we started with just the Event flow, then move onto a certain subset of DOM (Node and Element), then onto a certain subset of HTML (HTMLElement).
Basically Observable is EventTarget and this is what it should be like.
All of the above mentioned classes are based on undom-ng and do not implement anything from happy-dom specifically, maybe I have added some functions from happy-dom but it's almost all undom-ng. The
No breaking changes mean all existing stuff works as is and all tests pass. As far as the new
Types are already pretty close to libdom.d.ts however not 1:1 but that can be fixed to some extent. Right now I have only focused on making it work. Types can be fixed eventually to some degree only. Although I am afraid you can't actually make types merge into libdom.d.ts because things like
It's possible for very basic classes like Node/Comment/CharacterData but not for I have already done a big favor by not breaking the core and making all tests pass, it should be relatively easier to review. So even if this is merged, it does not affect anyone directly. There is indeed no hurry but I think there's not much benefit or use of just adding Events/EventTarget to core when everything else isn't there. Ultimately we will have to sit down and review big PRs if we want big and better changes for the foreseeable future in Core at some point, otherwise it stays where it is forever. This PR is still a draft, of course a lot can be improved here and there. But it gives us a good starting point to work on DOM for NativeScript. Ultimately even with a broken down implementation of DOM in multiple PRs, the goal is the same, to have a DOM in NativeScript. Initially i think my goal is to get frameworks working and functional same as dominative where i can mark this somewhat ready. |
ammarahm-ed
force-pushed
the
dom-in-core
branch
from
2ee74fd
to
6cb5d8a
Compare
- Adds basic support for DOMTokenList - Fixes automatic addition of cssType when registering elements with DOM
This is my humble attempt to conform core to DOM &
EventTarget.Here's what's added:
HTMLElement&EventTargetObservablenow conforms toEventTargetwithout breaking current observable implementation.Working web frameworks with minimum configuration:
Most of these will be based on exisiting implementations with dominative.
Other APIs to add
This is largely based on undom-ng & dominative by @ClassicOldSong and happy-dom.
This is more about conformance than performance. Although i have tried to keep everything simple and lean and quite happy about the results.
The Observable changes by being backwards compatible result in a some overhead of course which is fixable by simply moving to
dispatchEventinstead ofnotifywhich is mostly because values are first set onEventDatathen they get defined on the DOMEventagain.Class initialization/Adding & removing listeners has same performance compared to current core. I'll add a benchmark comparison table soon.
The ultimate end-developer using a web framework will be exposed to DOM types from @nativescript-dom/type which we can add in core eventually.
Benchmarks
Without DOM:
With DOM:
Update:
After adding some optimizations, domless
notifyandnotifycalls with DOM perform the same:PR Checklist
What is the current behavior?
There is no DOM in core
What is the new behavior?
There is DOM in core
No breaking changes introduced.