samuelgozi / firebase-auth-lite

A lightweight firebase auth alternative for the browser
MIT License
119 stars 19 forks source link

Fix docs and update dependencies #37

Closed DairAidarkhanov closed 4 years ago

DairAidarkhanov commented 4 years ago

Refactor code for existing users before releasing v1. Featuring #38

issue-label-bot[bot] commented 4 years ago

Issue-Label Bot is automatically applying the label feature_request to this issue, with a confidence of 0.91. Please mark this comment with :thumbsup: or :thumbsdown: to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

samuelgozi commented 4 years ago

I'm currently investing most of my (free) time on the Firestore library. After relasing it's v1 I'll switch to finishing the refactor of this library, which could be seen in branch typescript.

A few things to note about how this library is written. As I outlined in the review of your code, this library is aggressively optimized for size and performance. Many of the short-circuiting and weird syntax is for those reasons. I decided to trade some readability with smaller bundles, and this decision was made because I don't think that most developers dive into the code of this kind of libraries, and in addition, the library is small enough to reason about even with this "burden".

Now, many of the other style changes you suggested are purely that. They are style preferences which In my opinion don't make the code more readable, and also add up to bigger bundles.

For example replacing:

if(x) return;

with:

if(x) {
  return;
}

In my humble opinion doesn't seem to make the code more readable, and at the same time takes more space. Also adding else statements for that matter. It is implied that everything outside of an if statement is an "else", and adding a code block for that adds size and complexity(because of scoping).

Thank you yet again for your good intentions. Many of your grammar changes are very welcome, and also some of your suggestions. Feel free to share your opinions on anything. And please open an issue before making changes, as I might be working some things already, and maybe some changes are not needed anyways(I've made the same mistake many times too).

DairAidarkhanov commented 4 years ago

Thank you! I made the changes for consistency's sake. You are welcome!

Edit: I am still looking for adding indexedDB adapter to the project and there is a problem with parsing json while getting user credentials from a storage. It is necessary to parse JSON from the localStorage data but is not needed when getting the data from the indexedDB storage as it stores JSON objects as is.

This is basically coupling to the implementation details. Maybe, there is an idea to decouple object parsing and data extraction? Nevertheless, it is a completely different issue.

samuelgozi commented 4 years ago

@aidarkhanov I think well leave the logic for parsing the data to the adapter itself. But all those changes should be done in the typescript branch. And I dont want to add any new dependencies to the project unless its 100% necessary so the adapter should implement the logic for working with indexedDB itself or be an external dependency altogether.

DairAidarkhanov commented 4 years ago

Closing the issue. Glad to help!

samuelgozi commented 4 years ago

Thanks for the help!