{"action":"created","issue":{"url":"https://api.github.com/repos/firebase/firebase-js-sdk/issues/17","repository_url":"https://api.github.com/repos/firebase/firebase-js-sdk","labels_url":"https://api.github.com/repos/firebase/firebase-js-sdk/issues/17/labels{/name}","comments_url":"https://api.github.com/repos/firebase/firebase-js-sdk/issues/17/comments","events_url":"https://api.github.com/repos/firebase/firebase-js-sdk/issues/17/events","html_url":"https://github.com/firebase/firebase-js-sdk/issues/17","id":229782972,"node_id":"MDU6SXNzdWUyMjk3ODI5NzI=","number":17,"title":"FR: Offline first support for PWAs (RTDB)","user":{"login":"Hollerweger","id":6900492,"node_id":"MDQ6VXNlcjY5MDA0OTI=","avatar_url":"https://avatars1.githubusercontent.com/u/6900492?v=4","gravatar_id":"","url":"https://api.github.com/users/Hollerweger","html_url":"https://github.com/Hollerweger","followers_url":"https://api.github.com/users/Hollerweger/followers","following_url":"https://api.github.com/users/Hollerweger/following{/other_user}","gists_url":"https://api.github.com/users/Hollerweger/gists{/gist_id}","starred_url":"https://api.github.com/users/Hollerweger/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Hollerweger/subscriptions","organizations_url":"https://api.github.com/users/Hollerweger/orgs","repos_url":"https://api.github.com/users/Hollerweger/repos","events_url":"https://api.github.com/users/Hollerweger/events{/privacy}","received_events_url":"https://api.github.com/users/Hollerweger/received_events","type":"User","site_admin":false},"labels":[{"id":610727796,"node_id":"MDU6TGFiZWw2MTA3Mjc3OTY=","url":"https://api.github.com/repos/firebase/firebase-js-sdk/labels/Component-Database","name":"Component-Database","color":"F9CD9E","default":false},{"id":641653033,"node_id":"MDU6TGFiZWw2NDE2NTMwMzM=","url":"https://api.github.com/repos/firebase/firebase-js-sdk/labels/Status:%20In%20Progress","name":"Status: In Progress","color":"cccccc","default":false},{"id":606552212,"node_id":"MDU6TGFiZWw2MDY1NTIyMTI=","url":"https://api.github.com/repos/firebase/firebase-js-sdk/labels/feature-request","name":"feature-request","color":"fbca04","default":false},{"id":626807074,"node_id":"MDU6TGFiZWw2MjY4MDcwNzQ=","url":"https://api.github.com/repos/firebase/firebase-js-sdk/labels/help-wanted","name":"help-wanted","color":"34ff2d","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":95,"created_at":"2017-05-18T19:40:56Z","updated_at":"2018-11-24T13:37:03Z","closed_at":null,"author_association":"NONE","body":"While the Firebase JS SDK has support for offline scenarios when the web app goes from online to offline it lacks offline first support.\r\nOffline first is a crucial part of PWAs and should be supported by the Firebase JS SDK directly."},"comment":{"url":"https://api.github.com/repos/firebase/firebase-js-sdk/issues/comments/441368464","html_url":"https://github.com/firebase/firebase-js-sdk/issues/17#issuecomment-441368464","issue_url":"https://api.github.com/repos/firebase/firebase-js-sdk/issues/17","id":441368464,"node_id":"MDEyOklzc3VlQ29tbWVudDQ0MTM2ODQ2NA==","user":{"login":"jsayol","id":2029586,"node_id":"MDQ6VXNlcjIwMjk1ODY=","avatar_url":"https://avatars1.githubusercontent.com/u/2029586?v=4","gravatar_id":"","url":"https://api.github.com/users/jsayol","html_url":"https://github.com/jsayol","followers_url":"https://api.github.com/users/jsayol/followers","following_url":"https://api.github.com/users/jsayol/following{/other_user}","gists_url":"https://api.github.com/users/jsayol/gists{/gist_id}","starred_url":"https://api.github.com/users/jsayol/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jsayol/subscriptions","organizations_url":"https://api.github.com/users/jsayol/orgs","repos_url":"https://api.github.com/users/jsayol/repos","events_url":"https://api.github.com/users/jsayol/events{/privacy}","received_events_url":"https://api.github.com/users/jsayol/received_events","type":"User","site_admin":false},"created_at":"2018-11-24T13:37:03Z","updated_at":"2018-11-24T13:37:03Z","author_association":"CONTRIBUTOR","body":"Hi there.\r\n\r\nI just remembered that back when I was implementing this last year I ran into an issue that could become a potential breaking change.\r\n\r\nSome background: when a new listener is attached with the current implementation, either via `.on()` or `.once()`, then [`SyncTree.addEventRegistration()`](https://github.com/firebase/firebase-js-sdk/blob/7a41fe71fd3effd71b61548546e290015f58456b/packages/database/src/core/SyncTree.ts#L317-L387) returns a list of events to be raised immediately (synchronously) based on the data that can already be found on the in-memory cache.\r\n\r\nBy adding persistence into the mix, now [`addEventRegistration()`](https://github.com/jsayol/firebase-js-sdk/blob/7abcd1ca06170cb60e8db4d310ab66ce2b95acaa/packages/database/src/core/SyncTree.ts#L396-L510) needs to account for the possibility that it might need to access the disk cache, which is asynchronous. This means that now it needs to return a Promise that resolves to that list of events instead.\r\n\r\nWith that in mind, take the following code from one of the current Transaction tests:\r\n\r\n```ts\r\nit('New value is immediately visible.', function() {\r\n  const node = getRandomNode() as Reference;\r\n  node.child('foo').transaction(function() {\r\n    return 42;\r\n  });\r\n\r\n  let val = null;\r\n  node.child('foo').on('value', function(snap) {\r\n    val = snap.val();\r\n  });\r\n  expect(val).to.equal(42);\r\n});\r\n```\r\n\r\nI've already modified several other tests that were directly inspecting the list of raised internal events synchronously, since that is not a real use case. But this example is different since people's code might be currently relying on this behavior. It's uncommon, but I'd say it's perfectly valid to do that if you *know* the value you're looking for has already been cached.\r\n\r\nIf you don't want to introduce a breaking change here, which is perfectly understandable, the only alternative I see would be to revert `addEventRegistration()` back to synchronously returning the list of events to be raised based only on the in-memory cache, and then asynchronously raise any events based on the disk cache. From a practical point of view, it would behave just as if new data had come from the server after attaching the listener.\r\n\r\nWould that be an acceptable solution?"}}
{"actor":{"display_login":"jsayol"}}