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

get() server data must be indexed #4796

Merged
merged 6 commits into from
May 24, 2021
Merged

get() server data must be indexed #4796

merged 6 commits into from
May 24, 2021

Conversation

jmwski
Copy link
Contributor

@jmwski jmwski commented Apr 15, 2021

If we're getting data from the server, it should be indexed with the query param index.

For the value event registration path, I think the query index is applied using IndexFilter::updateFullNode on the filter attached to the view that the listener sets up front.

Experimentally, even when the query results are already cached (even if they don't have the same path index), as in:

    const things = db.ref().child("things");

    await things.remove();

    things.push().set({ "thing": 1 });
    things.push().set({ "thing": 2 });


    await new Promise((resolve, _) => {
        things.on("value", function (results) {
            resolve();
        })
    });

    things.orderByChild("thing").get().then(function (results) {
        results.forEach(function (node) {
            console.log(JSON.stringify(node));
        });
    }).catch(function (err) {
        console.log("There's an error: " + err);
    })

get() will add the new path index to the CacheNode's index map. It's only in the case where data comes in from the server for the first time that we'd see this issue.

Side note: Looking at repoOnDataUpdate I also noticed that we should probably be calling repoRerunTransactions when server data comes in for the first time (though I'm not certain here).

@changeset-bot
Copy link

changeset-bot bot commented Apr 15, 2021

🦋 Changeset detected

Latest commit: 0ef4c5b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@firebase/database Patch
firebase Patch
@firebase/rules-unit-testing Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Apr 15, 2021

Size Analysis Report

Affected Products

No changes between base commit (997040a) and head commit (750e268).

Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

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

Please also add a changelog

packages/database/src/core/Repo.ts Show resolved Hide resolved
@jmwski jmwski changed the title get() server data must be indexed + transaction fix get() server data must be indexed May 11, 2021
@jmwski jmwski assigned schmidt-sebastian and unassigned jmwski May 11, 2021
.changeset/rotten-wombats-shop.md Outdated Show resolved Hide resolved
@google-oss-bot
Copy link
Contributor

google-oss-bot commented May 11, 2021

Binary Size Report

Affected SDKs

  • @firebase/database

    Type Base (997040a) Head (750e268) Diff
    browser 296 kB 296 kB +41 B (+0.0%)
    esm2017 265 kB 265 kB +41 B (+0.0%)
    main 299 kB 299 kB +41 B (+0.0%)
    module 296 kB 296 kB +41 B (+0.0%)
  • @firebase/database-exp

    Type Base (997040a) Head (750e268) Diff
    browser 246 kB 246 kB +41 B (+0.0%)
    main 278 kB 278 kB +41 B (+0.0%)
    module 246 kB 246 kB +41 B (+0.0%)
  • firebase

    Type Base (997040a) Head (750e268) Diff
    firebase-database.js 187 kB 187 kB +37 B (+0.0%)
    firebase.js 889 kB 889 kB +37 B (+0.0%)

Test Logs

@jmwski jmwski merged commit 2a5039e into master May 24, 2021
@jmwski jmwski deleted the jw/get-index-fix branch May 24, 2021 14:34
@google-oss-bot google-oss-bot mentioned this pull request May 25, 2021
@firebase firebase locked and limited conversation to collaborators Jun 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants