Skip to content

ComponentProvider v2 #2889

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Apr 9, 2020
Merged

ComponentProvider v2 #2889

merged 8 commits into from
Apr 9, 2020

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented Apr 9, 2020 •
edited
Loading

So... obviously the component provider design broke the other PR I had in flight. In #2879, I rely on the ability to provide a custom persistence implementation in the SpecTest that can inject failures. With the ComponentProvider as implemented, I would have to build up all components from scratch. It would be nice if I only needed to change the persistence layer.

I pushed three versions:

  • e01a7c3 is cleaner, but increases size by 580b (full build) / 973b (memory)
  • 9725015 moves some shared code to ComponentBuilder. This only helps the full build (+241b instead of 580b)
  • f1d4a49 gets rid of the abstract class and has a size increase of 311b (full) / 725b (memory)

I personally prefer the last option (the diff of this PR).

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Apr 9, 2020 •
edited
Loading

Binary Size Report

Affected SDKs

  • @firebase/firestore

    Type Base (7fa3d03) Head (18730fc) Diff
    browser 252 kB 252 kB -47 B (-0.0%)
    esm2017 199 kB 199 kB +20 B (+0.0%)
    main 486 kB 486 kB +66 B (+0.0%)
    module 250 kB 250 kB -13 B (-0.0%)

  • @firebase/firestore/memory

    Type Base (7fa3d03) Head (18730fc) Diff
    browser 198 kB 199 kB +774 B (+0.4%)
    esm2017 156 kB 156 kB +375 B (+0.2%)
    main 374 kB 376 kB +1.33 kB (+0.4%)
    module 197 kB 197 kB +752 B (+0.4%)

  • firebase

    Type Base (7fa3d03) Head (18730fc) Diff
    firebase-firestore.js 294 kB 294 kB +12 B (+0.0%)
    firebase-firestore.memory.js 241 kB 242 kB +752 B (+0.3%)
    firebase.js 828 kB 828 kB +9 B (+0.0%)

Test Logs

@schmidt-sebastian schmidt-sebastian force-pushed the mrschmidt/componentsv2 branch 2 times, most recently from c141136 to f1d4a49 Compare April 9, 2020 16:39
@wilhuff
Copy link
Contributor

wilhuff commented Apr 9, 2020

I wonder if there's an approach in between the two--what if you made the base class the memory-only version and then made the IndexedDB configuration extend that? That way all the common stuff could stay common but the memory-only case wouldn't pay the penalty of the extra abstract class?

@schmidt-sebastian
Copy link
Contributor Author

Updated with your suggestion.

wilhuff
wilhuff approved these changes Apr 9, 2020
View reviewed changes
Copy link
Contributor

@wilhuff wilhuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wilhuff wilhuff assigned schmidt-sebastian and unassigned wilhuff Apr 9, 2020
@schmidt-sebastian schmidt-sebastian merged commit ce3addb into master Apr 9, 2020
@firebase firebase locked and limited conversation to collaborators May 10, 2020
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/componentsv2 branch July 9, 2020 17:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wilhuff wilhuff wilhuff approved these changes

Assignees

@schmidt-sebastian schmidt-sebastian

Labels
api: firestore
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@schmidt-sebastian @google-oss-bot @wilhuff