search

samuelgozi / firebase-auth-lite

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

Refactor code and update dependencies #36

Closed DairAidarkhanov closed 4 years ago

DairAidarkhanov commented 4 years ago

Changes to facilitate the indexedDB storage option. Featuring the issue #37

samuelgozi commented 4 years ago

Thanks for taking the time to help me out. I would like to suggest that next time open an issue with suggested changes before working on them, because some of them might not be accepted, and some might be worked on already or deprecated and the work might be for nothing.

Some of the changes you made I don't accept:

It is written that way in order to save space. Even though it might be more readable to write it the other way, it's a trade that needs to be thought of, And the change was made already in the branch typescript.

Extracting it into a different file is not necessary. The reason for that is that at the time of writing most websites use bundlers, and adding the boilerplate for polyfilling modules takes more space than the code itself. And since right now there is only one file I try to keep it that way until a few are needed.

The other changes to grammar are good, but I rather not work on this branch other than bug fixes. All important changes should be done to the branch typescript which is a refactor of the project and will be merged when I finish it.

Thank you very much for taking the time to help, I really appreciate it!

DairAidarkhanov commented 4 years ago

Great! Regarding imports, I think you are right that we should keep it simple.

But eventually we should package the production code in "dist" folder with ES module and CommonJS versions. For that, Rollup is a great option because it can compile, minify, link and tree-shake the final code.

It will be necessary when the package migrates over to typescript.

Meanwhile, I can revert importing separate file for the bug fixes to propagate for current users of the package.

samuelgozi commented 4 years ago

I'm trying to avoid bundling the code for the developer because it can cause many errors and add unnecessary code. I would like to provide a clean ES Module and let the dev deal with the environment/bundler.

DairAidarkhanov commented 4 years ago

@samuelgozi Ok, I have removed storage.js file and I think it is fine to merge.

DairAidarkhanov commented 4 years ago

I have put up code with original structure. I've left async api function because it works the same and trims the code down a little.

Edit: this pull request turned into a mess. I better create a new pr.