Nx Cloud Report

Attention: 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:

Successfully synced application org.nativescript.UnitTestApp on device emulator-5554.
  JS: Test: --- [TAB-VIEW-NAVIGATION.testBackNavigationToTabViewWithNestedFramesShouldWork] OK, duration: 961.95
  JS: Test: --- [TAB-VIEW-NAVIGATION.testLoadedAndUnloadedAreFired_WhenNavigatingAwayAndBack] OK, duration: 412.85
  JS: Test: --- [TAB-VIEW-NAVIGATION.testWhenNavigatingBackToANonCachedPageContainingATabViewWithAListViewTheListViewIsThere] OK, duration: 641.98
  JS: Test: TAB-VIEW-NAVIGATION COMPLETED for 2018.64 BACKSTACK DEPTH: 1
  JS: Test:
  JS: === ALL TESTS COMPLETE ===
  JS: 3 OK, 0 failed
  JS: DURATION: 2020.16 ms
  JS: === END OF TESTS ===
  JS: Test: Tests EOF!
  JS: image
  JS: Test: START IMAGE TESTS.
  JS: Test: --- [IMAGE.test_DimensionsAreRoundedAfterScale] FAILED: com.tns.NativeScriptException: Calling js method destroyItem failed
  JS: Error: java.lang.IllegalStateException: Cannot detach Fragment attached to a different FragmentManager. Fragment FragmentBase_bundle_73170_28_TabFragmentImplementation{147fe05} (332c632d-3fef-4586-b428-47ee23f29de0 id=0x2 tag=android:viewpager:2:0) is already attached to a FragmentManager., Stack: setAdapterItems(file: src/packages/core/ui/tab-view/index.android.ts:683:23)
  JS:   at onUnloaded(file: src/packages/core/ui/tab-view/index.android.ts:606:7)
  JS:   at (file: src/packages/core/ui/core/view-base/index.ts:546:69)
  JS:   at callFunctionWithSuper(file: src/packages/core/ui/core/view-base/index.ts:535:2)
  JS:   at callUnloaded(file: src/packages/core/ui/core/view-base/index.ts:546:7)
  JS:   at unloadView(file: src/packages/core/ui/core/view-base/index.ts:730:8)
  JS:   at _removeViewCore(file:///data/data/or...
  JS: Test: --- [IMAGE.test_Image_Members] OK, duration: 0.23
  System.err: An uncaught Exception occurred on "main" thread.
  System.err: Calling js method onPause failed
  System.err: TypeError: Cannot read property 'setDrawingCacheEnabled' of null
  System.err:
  System.err: StackTrace:
  System.err: TabFragmentImplementation.loadBitmapFromView(file:///data/data/org.nativescript.UnitTestApp/files/app/bundle.js:73224:14)
  System.err:   at TabFragmentImplementation.onPause(file:///data/data/org.nativescript.UnitTestApp/files/app/bundle.js:73212:42)
  System.err:   at com.tns.Runtime.callJSMethodNative(Native Method)
  System.err:   at com.tns.Runtime.dispatchCallJSMethodNative(Runtime.java:1302)
  System.err:   at com.tns.Runtime.callJSMethodImpl(Runtime.java:1188)
  System.err:   at com.tns.Runtime.callJSMethod(Runtime.java:1175)
  System.err:   at com.tns.Runtime.callJSMethod(Runtime.java:1153)
  System.err:   at com.tns.Runtime.callJSMethod(Runtime.java:1149)
  System.err:   at org.nativescript.widgets.FragmentBase_bundle_73170_28_TabFragmentImplementation.onPause(Unknown Source:15)
  System.err:   at androidx.fragment.app.Fragment.performPause(Fragment.java:3305)
  System.err:   at androidx.fragment.app.FragmentStateManager.pause(FragmentStateManager.java:631)
  System.err:   at androidx.fragment.app.FragmentStateManager.moveToExpectedState(FragmentStateManager.java:293)
  System.err:   at androidx.fragment.app.FragmentStore.moveToExpectedState(FragmentStore.java:113)
  System.err:   at androidx.fragment.app.FragmentManager.moveToState(FragmentManager.java:1433)
  System.err:   at androidx.fragment.app.FragmentManager.dispatchStateChange(FragmentManager.java:2977)
  System.err:   at androidx.fragment.app.FragmentManager.dispatchPause(FragmentManager.java:2913)
  System.err:   at androidx.fragment.app.Fragment.performPause(Fragment.java:3298)
  System.err:   at androidx.fragment.app.FragmentStateManager.pause(FragmentStateManager.java:631)
  System.err:   at androidx.fragment.app.FragmentStateManager.moveToExpectedState(FragmentStateManager.java:293)
  System.err:   at androidx.fragment.app.FragmentManager.executeOpsTogether(FragmentManager.java:1899)
  System.err:   at androidx.fragment.app.FragmentManager.removeRedundantOperationsAndExecute(FragmentManager.java:1817)
  System.err:   at androidx.fragment.app.FragmentManager.execPendingActions(FragmentManager.java:1760)
  System.err:   at androidx.fragment.app.FragmentManager$5.run(FragmentManager.java:547)
  System.err:   at android.os.Handler.handleCallback(Handler.java:938)
  System.err:   at android.os.Handler.dispatchMessage(Handler.java:99)
  System.err:   at android.os.Looper.loopOnce(Looper.java:201)
  System.err:   at android.os.Looper.loop(Looper.java:288)
  System.err:   at android.app.ActivityThread.main(ActivityThread.java:7839)
  System.err:   at java.lang.reflect.Method.invoke(Native Method)
  System.err:   at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
  System.err:   at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1003)
  JS: NativeScriptError: Error: Calling js method onPause failed
  JS: TypeError: Cannot read property 'setDrawingCacheEnabled' of null

It is related to the Observable changes. Something very specific probably? If i add a couple of checks, then everything is ok. Basically this.owner.nativeViewProtected is null when onPause gets called:

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
Could we have benchmarks for events and also view tree changes ? If there is no hit and it helps with flavors integration then it is a great addition

I am personally worried about performances hit of this PR.

This PR is not about performance but conformance to "something" especially the DOM. Performance can be further optimized.

Especially when the change is not really needed in the sense it does not add more to N

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 DOM is very basic for web frameworks and should be a part of NativeScript core instead of living as an extra layer above it which most framework implementations do currently. I think a more pointless thing in core is the XML flavor that should have been outside the core. And if an xml flavor can live in core, I think it's okay for NativeScript core to BE the DOM so that all flavors can function the same. We can have things like Shadow DOM, plugins that we write once and use anywhere with Lit html & much more. It does add a lot and also puts the NativeScript core into the right direction. Because right now it does not conform to anything which is painful.

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 notify functions as I mentioned above. The DOM implementation conforms to EventTarget 1:1 which has some overhead but if you use dispatchEvent directly, that overhead is removed automatically.

This allows us to slowly make core like DOM and conform EventTarget in phases in plugins & core itself. It removes a lot of overhead of maintaining each flavor a lot like what dominative does.

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.

@ClassicOldSong @farfromrefug

Benchmarks:

Without DOM:

# 1

  JS: new FlexboxLayout() x 39,037 ops/sec ±4.34% (37 runs sampled)
  JS: view.notify (20 Listeners) x 1,110,038 ops/sec ±2.42% (60 runs sampled)
  JS: view.addChild (shallow) x 860 ops/sec ±8.01% (42 runs sampled)
  JS: view.addChild (deep) x 263 ops/sec ±31.06% (46 runs sampled)

# 2

  JS: new FlexboxLayout() x 41,885 ops/sec ±5.71% (39 runs sampled)
  JS: view.notify (20 Listeners) x 1,083,994 ops/sec ±5.60% (56 runs sampled)
  JS: view.addChild (shallow) x 870 ops/sec ±7.84% (42 runs sampled)
  JS: view.addChild (deep) x 258 ops/sec ±33.50% (47 runs sampled)

With DOM:

# 1

  JS: document.createElement x 31,484 ops/sec ±53.77% (32 runs sampled)
  JS: new View() x 25,338 ops/sec ±90.88% (46 runs sampled)
  JS: element.dispatchEvent (20 Listeners) x 1,005,212 ops/sec ±2.06% (58 runs sampled)
  JS: view.notify (20 Listeners) x 600,275 ops/sec ±1.24% (57 runs sampled)
  JS: element.appendChild (shallow) x 841 ops/sec ±7.96% (43 runs sampled)
  JS: view.addChild (shallow) x 880 ops/sec ±9.67% (36 runs sampled)
  JS: element.appendChild (deep) x 155 ops/sec ±100.27% (38 runs sampled)
  JS: view.addChild (deep) x 316 ops/sec ±3.38% (54 runs sampled)

# 2 reversed sequence of calls

  JS: new View() x 33,129 ops/sec ±48.56% (34 runs sampled)
  JS: document.createElement x 25,208 ops/sec ±97.78% (48 runs sampled)
  JS: view.notify (20 Listeners) x 621,300 ops/sec ±1.21% (56 runs sampled)
  JS: element.dispatchEvent (20 Listeners) x 1,079,043 ops/sec ±1.49% (55 runs sampled)
  JS: view.addChild (shallow) x 881 ops/sec ±6.82% (45 runs sampled)
  JS: element.appendChild (shallow) x 857 ops/sec ±10.77% (34 runs sampled)
  JS: view.addChild (deep) x 303 ops/sec ±3.52% (53 runs sampled)
  JS: element.appendChild (deep) x 308 ops/sec ±8.91% (22 runs sampled)

As I mentioned before, using dispatchEvent performs many times faster than using notify with DOM. The overhead is obvious since notify is backward compatible so it has to do more work to fire the event. But still, 600K ops/sec is a lot with 20 listeners attached at once, it really depends ultimately how what you do inside the event callback that would affect performance. When you using dispatchEvent performance is same as domless core.

View creation calls seem a bit slow at first, but if you look at appendChild calls, you will notice that actual tree operations are the same with real views being created can be same or better sometimes.

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.
Did you run those on an emulator or an a real device? Can those benchmarks be run with that PR ? I am asking because emulator do not reflect at all the reality of running performance benchmarks on real device (especially low end devices).

Now about the result and your comment.

• in your with dom benchmark you dont seem to be testing new FlexboxLayout() like you do in without dom. Would that give similar result than without dom? I understand that createElement would be slower but i am more worried about the changes occuring in the core itself. If it does not make a difference it is great!
• notify this one seems a lot more worring to me. We should remember than notify is not only used in tap events or things like that. notify is also the main (only?) tool we have at our disposal to pass on data from a plugin to user space. For example we use it for real time sensor datas (nativescript-accelerometer, @nativescript-community/sensors) or for custom drawing for views which can be drawing at high fps or for animations (used in @nativescript-community/ui-canvas and all its derivated plugins, @nativescript/canvas might even be using it @triniwiz ?). And there notify performance is critical to ensure we keep real time updates as fast as possible. I understand dispatchEvent could be the solution but it would require all plugins to update and be a breaking change. I think @NathanWalker once metioned that "dom events" could be an opt-in feature? maybe this could be a solution to bring the feature in while ensuring N remains as fast and competitive with other platforms as it can be.

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.
If a client refuse to use N because it is not competitive with other frameworks (and performance is a key feature, i have ported many RN apps because they were simply non performant), then no dev (yes i omit the OSS apps) will ever be able to use N or its great (and yes i agree it would be great) DOM compliance features.

@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.

@ammarahm-ed thanks for those benchmarks.
Did you run those on an emulator or an a real device? Can those benchmarks be run with that PR ? I am asking because emulator do not reflect at all the reality of running performance benchmarks on real device (especially low end devices).

On real device

Now about the result and your comment.

• in your with dom benchmark you dont seem to be testing new FlexboxLayout() like you do in without dom. Would that give similar result than without dom? I understand that createElement would be slower but i am more worried about the changes occuring in the core itself. If it does not make a difference it is great!

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 this one seems a lot more worring to me. We should remember than notify is not only used in tap events or things like that. notify is also the main (only?) tool we have at our disposal to pass on data from a plugin to user space. For example we use it for real time sensor datas (nativescript-accelerometer, @nativescript-community/sensors) or for custom drawing for views which can be drawing at high fps or for animations (used in @nativescript-community/ui-canvas and all its derivated plugins, @nativescript/canvas might even be using it @triniwiz ?). And there notify performance is critical to ensure we keep real time updates as fast as possible. I understand dispatchEvent could be the solution but it would require all plugins to update and be a breaking change. I think @NathanWalker once metioned that "dom events" could be an opt-in feature? maybe this could be a solution to bring the feature in while ensuring N remains as fast and competitive with other platforms as it can be.

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.

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.

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.

If a client refuse to use N because it is not competitive with other frameworks (and performance is a key feature, i have ported many RN apps because they were simply non performant), then no dev (yes i omit the OSS apps) will ever be able to use N or its great (and yes i agree it would be great) DOM compliance features.

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 notify.

JS: view.notify (20 Listeners) x 1,068,513 ops/sec ±0.86% (62 runs sampled)
JS: element.dispatchEvent (20 Listeners) x 1,060,945 ops/sec ±1.19% (59 runs sampled)

@ammarahm-ed that is an amazing news!
Sorry for pushing you yo give those numbers. Really happy with them now! Great job
Will try to test your PR in the days to come

@ammarahm-ed It might be a good idea to add self property to window, too. Might prove useful when we extend Worker functionality in the future.
https://developer.mozilla.org/en-US/docs/Web/API/Window/self
This requires implementation of WindowProxy but we could add it as is for now?
If we face issues like circular reference, we could perhaps set it as non-enumerable property.

@ammarahm-ed It might be a good idea to add self property to window, too. Might prove useful when we extend Worker functionality in the future.
https://developer.mozilla.org/en-US/docs/Web/API/Window/self
This requires implementation of WindowProxy but we could add it as is for now?
If we face issues like circular reference, we could perhaps set it as non-enumerable property.

Hmm 🤔 unless we have multiple windows in the app, we might do fine with a single window object in global being referenced as self.

@shirakaba

The inheritance chain now goes: Viewbase > HTMLElement > Element > ParentNode > Node > Observable > EventTarget.

Basically Observable is EventTarget and this is what it should be like.

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).

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 EventTarget/Observable changes are my own. Stuff like window & document etc follow happy-dom and other helper classes are from happy-dom too. Initially I didn't expect to add stuff from happy-dom but I think I have added some new classes from there now. I can't add comment and links on each function but will add a README.md to mention happy-dom/undom-ng and it is derived from both at this point largely.

A new class named Event is 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.

No breaking changes mean all existing stuff works as is and all tests pass. As far as the new Event class is concerned, that is in line with the DOM implementation & how EventTarget works. For exisiting notify calls, they work directly in core without the Event class unless the View is a DOM element.

I think writing a DOM implementation is all for nothing if the DOM types don't derive exactly from libdom.d.ts.

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 Element & HTMLElement implement only a small subset of what a libdom.d.ts HTMLElement/Element implement (Adding lots of web related stuff which we might never add). Yeah, we can conform what functions/props are implemented to libdom.d.ts but to make it work seemlessly from the outside, the developer must simply depend on libdom.d.ts instead of core for DOM related types, otherwise it's not possible to do that unless you implement HTMLElement/Element 100%.

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.

It's possible for very basic classes like Node/Comment/CharacterData but not for Element/HTMLElement/Document/Window. Also I think our focus isn't conforming to libdom.d.ts, just making DOM work directly. Types conformance is a separate problem and not a simple one in this case.

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.