magiccodingman / Magic.IndexedDb

Use IndexedDb in Blazor like LINQ!
MIT License
28 stars 8 forks source link

A little suggestion: use "import" to load the js scripts #1

Closed yueyinqiu closed 1 year ago

yueyinqiu commented 1 year ago

An amazing project! Actually, this might be the best IndexedDb library for blazor I have ever found. Most libraries depend on TG.Blazor.IndexedDB, which has no update for 3 years and is still in preview. And this project is also very young, which means it might be much more easy to have breaking changes currently. So I feel very exciting to find the project at this time.

And here is my first suggestion, just a little suggestion: using "import" to load the js scripts.

Currently it's running window.magicBlazorDB = ... in js in advance, and then using _jsRuntime.InvokeAsync<TResult>("window.magicBlazorDB.xxxxxxx") in c# to invoke the functions. This method could work, but it requires the users to manually write <script src="_content/Magic.IndexedDb/magicDB.js"></script>. More importantly, it will occupy the property window.magicBlazorDB. Although hardly another packages will also use this property, it is a hidden trouble.

Instead, using "import" could avoid the above problem. For this project, we can use var module = await JS.InvokeAsync<IJSObjectReference>("import", "./_content/Magic.IndexedDb/magicDB.js") to get the module, and then use module.InvokeAsync<TResult>("xxxxx") to invoke the functions. Of course, magicDB.js should be rewrited as a module at the same time, like:

import ... from "./dexie.min.js"; // also directly import dexie here
export function createDb (dotnetReference, transaction, dbStore) {
    ...
}
export function ...
...

In this way, it don't need to assign any value to window.magicBlazorDB, and users can get rid of writing the lines like <script src="_content/Magic.IndexedDb/magicDB.js"></script>. I think this could make the package better. Finally, thanks for your great work.

yueyinqiu commented 1 year ago

another suggestion now is to deal with the nullable reference.... 102 warnings about it.... this used to be written in lower dotnet versions?

magiccodingman commented 1 year ago

@yueyinqiu Thanks for the feedback, I appreciate it! This is my first open source project, my first time dealing with IndexedDb, and I'm honestly not the best with JavaScript. But I really wanted some package like this for my work, so I bit the bullet and wrote this in roughly a week.

And the no updates for 3 years is another big reason I wanted to write this. Many of the other packages didn't have updates for a long time so they're more likely to break like you said. Which is why I wanted a Dexie.Js wrapper so that even if I drop off the face of the earth, the wrapper will always work and Dexie.JS will continue to get updates without me. Additionally, this method allowed me to write a lot of code without needing to be an expert in IndexedDb Js. Creating the Where method though was a huge pain haha. But I have a lot of future features I want to add and performance improvements still.

But I honestly had no idea you could import scripts like that. But now that you mention it, since NET 6 release, I haven't seen others require that script tag anymore, so that makes sense! I definitely will add that as I don't like requiring the script tag either. Though, it makes me wonder if I should keep the Dexie.Js for the user to add the script tag or not so they can up the versions if they choose. Or I guess if I drop off the planet, someone else can update the package/code. You have an opinion on leaving Dexie.JS as a script tag or importing it?

Also, I definetly will go back and remove the null references. I should have done that from the start. Thanks a ton though for the feedback, I genuinely appreciate it. I was hoping to get some eyes on the project so I could get suggestions as I am not an expert on IndexedDb at all. I really was hoping to get the help of the community if possible :)

magiccodingman commented 1 year ago

another suggestion now is to deal with the nullable reference.... 102 warnings about it.... this used to be written in lower dotnet versions?

I just pushed an update (not to nuget package yet) that fixes all 60+ null reference warnings and I also accidentally broke the AddRange last update and just repaired that as well.

yueyinqiu commented 1 year ago

Haha, you're much better than me. In fact I have also tried to write a IndexedDb package, and my aim was just to make a simple wrapper for it but still failed. It's really a hard work.

And about dexie.js, my suggestion is to regard it as an implementation detail.

Comparing to drop off the planet -> no more update -> users can manually choose the new version, I'm much more worried about that if there is a breaking change of dexie.js, this chain will become users are using the new versions of dexie.js -> undefined, error, ... -> what a shit package (╯‵□′)╯ (not seriously) even if the package is still updating but just a bit behind dexie.js (and it must be).

Meanwhile, for most users, there are no reasons for them to specify the version of dexie.js. And if you are really afraid of 'dropping off the planet', just believe that there will be someone else taking over your great work! ;)

magiccodingman commented 1 year ago

@yueyinqiu You're 100% right. Allowing different versions of dexie not part of the package is more likely to create breaking changes since Dexie is still updated consistently. So it's likely they depreciate code as they've already done in the past as I've skimmed through much of their documentation and I've seen that before. So, you've convinced me! I appreciate the suggestions/advice. I'm all ears. I'll be adding the import hopefully tonight when I have a bit to sit down.

I've been adding to this package as I run into things since I built this to accomplish required goals I have on a separate project. So as my side project project moves alone, I'm dragging this project with it :) I've been building out some cool SignalR to API sync functionalities and stuff like that as well. I'm honestly really glad someone took a look at the code. It was a really fun project that I wanted to talk to others about it :D Thank you!

magiccodingman commented 1 year ago

@yueyinqiu Update released where you no longer need the script tags unless you use the Nuget packages version 1.0.3 or lower. I've yet to release 1.0.4 on Nuget, but I'll be able to later. Thanks again for the feedback, cheers!