Skip to content
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

Lazy load JS modules #3159

Merged
merged 9 commits into from
Nov 29, 2021
Merged

Lazy load JS modules #3159

merged 9 commits into from
Nov 29, 2021

Conversation

stsrki
Copy link
Collaborator

@stsrki stsrki commented Nov 24, 2021

closes #3150

Adds the lazy load support for JS modules. It should work better when running along with the async code.

@stsrki stsrki requested a review from David-Moreira November 24, 2021 13:12
@David-Moreira
Copy link
Contributor

I don't have time to test this today. But this seems equivalent to the previous version? Even if you had the Lazy, it should be working the same? Am I missing smthg?

@stsrki
Copy link
Collaborator Author

stsrki commented Nov 24, 2021

I don't have time to test this today. But this seems equivalent to the previous version? Even if you had the Lazy, it should be working the same? Am I missing smthg?

It was a guess since I cannot reproduce it.

@stsrki stsrki changed the base branch from master to rel-0.9.5 November 25, 2021 09:55
@stsrki
Copy link
Collaborator Author

stsrki commented Nov 25, 2021

I think my last comment where I check the button element is not null has fixed the issue. But I still think we should keep the Lazy loading changes. @David-Moreira Do you agree?

@David-Moreira
Copy link
Contributor

I think it's okay to deploy it with either of the ways. But Let me do some testing later today. Unless you want to go with a release right away.

@stsrki
Copy link
Collaborator Author

stsrki commented Nov 25, 2021

I will rather wait. If all is good then I will release it tomorrow.

@David-Moreira
Copy link
Contributor

David-Moreira commented Nov 25, 2021

We've gone back to have race conditions on the js calls.
If you switch very quickly between pages you'll eventually get it. (I'm guessing it might be more noticeable on worst connections on server or slower processing devices on wasm)

I don't think it's anything to do with the modules, but I believe this might be because we ended up fixing Dispose? Since it's now calling dispose correctly... now these race conditions are back.
To solve this:

  • Maybe null checking every dotnet object reference in js?
  • Delay dotnet object reference disposal by a few ms?
  • Is this such a low happening that we could swallow the exception?
  • Any "Cancellable work" patterns (similar to async Cancel token) out there that we can do in js?
  • Would not explicitly setting dotNetObjectRef = null; be helpful? Since the GC should eventually get rid of it anyway, null or not null. (At least under normal circumstances, if it thinks no component has a hold of it.)

If you figure it out in the meantime great! Otherwise I can help you on the weekend.

image
image
image

@stsrki
Copy link
Collaborator Author

stsrki commented Nov 26, 2021

I tried to fast switch between pages but maybe my PC is faster.


  • Maybe null checking every dotnet object reference in js? Yes, we can do that.
  • Delay dotnet object reference disposal by a few ms? NO, I don't want this to slow down the app in any way if possible.
  • Is this such a low happening that we could swallow the exception? What do you mean? try catch block?
  • Any "Cancellable work" patterns (similar to async Cancel token) out there that we can do in js? Really don't know 🤔
  • Would not explicitly setting dotNetObjectRef = null; be helpful? Since the GC should eventually get rid of it anyway, null or not null. (At least under normal circumstances, if it thinks no component has a hold of it.) We can set them to null if we didn't already.

@David-Moreira
Copy link
Contributor

Some pages are easier. Try between table / forms pages. I tried with blazor.server so there shouldn't be much diff in processing between our pcs? It's not like my pc is a stone! 😅

@stsrki
Copy link
Collaborator Author

stsrki commented Nov 26, 2021

I'm still unable to reproduce. Clicking as fast as I can between pages. Even enabled Slow 3G.

image

@David-Moreira
Copy link
Contributor

  • Maybe null checking every dotnet object reference in js? Yes, we can do that.
    R: OK.
    Delay dotnet object reference disposal by a few ms? NO, I don't want this to slow down the app in any way if possible.
    Is this such a low happening that we could swallow the exception? What do you mean? try catch block?
    R: YEA
    Any "Cancellable work" patterns (similar to async Cancel token) out there that we can do in js? Really don't know 🤔
    R: haha
    Would not explicitly setting dotNetObjectRef = null; be helpful? Since the GC should eventually get rid of it anyway, null or not null. (At least under normal circumstances, if it thinks no component has a hold of it.) We can set them to null if we didn't already.
    R: Oh, I meant removng the dotNetObjectRef = null instead.

@stsrki
Copy link
Collaborator Author

stsrki commented Nov 28, 2021

@David-Moreira So what did we agree on in the end. This is OK to merge?

@stsrki stsrki merged commit 8a7cd8a into rel-0.9.5 Nov 29, 2021
@stsrki stsrki deleted the rel-0.9.5-modules-loading branch November 29, 2021 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Button Issue Migrating 0.9.4.4 on RC 1 to 0.9.5 on .NET 6.0 RTM
2 participants