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

Proposal for community updates to target 2.15 release #522

Closed
mileszim opened this issue Aug 1, 2017 · 21 comments
Closed

Proposal for community updates to target 2.15 release #522

mileszim opened this issue Aug 1, 2017 · 21 comments
Assignees

Comments

@mileszim
Copy link

mileszim commented Aug 1, 2017

Through researching several continuing issues in both emberjs/data and emberfire, along with the rise of @rmmmp's emberfire-utils package to help alleviate these issues, I think it is time for a larger refactor.

With the target being emberjs/data 2.15, which includes a significant improvement over 2.14 and previous (synchronous findOrCreate styles, entire changes in event APIs like recordWillUnload no longer existing at the adapter level), as well as the monkey patches necessary for ember 1 commented in the emberfire code, there is a lot of room for good cleanup.

I am offering my help, and would like to see if we can integrate some of the improvements from emberfire-utils as well. We'll need to start by triaging which current issues are a) emberfire, b) emberjs/data, and which are fixed now. New problems are more likely emberjs/data.

Along with bug fixes, we can simplify and remove a lot of legacy code with the current state of emberjs and emberjs/data, like the monkeypatches. We'll want to triage all of those areas too.

I will update this issue with a checklist of things we can improve later today.

@mileszim
Copy link
Author

mileszim commented Aug 1, 2017

@elgordino what are your thoughts on adding your gist to the list here? it might make more sense as a separate PR, but I'm triaging a few things and this is where we should discuss :)

@mikkopaderes
Copy link
Contributor

Just laying out here what I've said in slack.

My original intention for emberfire-utils was to do experimentations outside of this addon and see what we can integrate in here. From being an experiment, my apps are now very dependent on it.

With that said, some of my addon's feature I believe are too opinionated as a default setup here. We'll need to cherry pick stuff (if any) that makes sense to be built-in here.

Lastly, I'm in favor of the goals laid out by @mileszim but Ember Data 2.15 as a target might not be realistic (it's happening in Aug 14th if on time). I think a better target is just to keep on refactoring if it won't break anything (if not, deprecate), then remove the deprecations in EmberFire 3.0. When will EmberFire 3.0 be released? Not sure, but I'd say when it's ready. 😁

@mileszim
Copy link
Author

mileszim commented Aug 2, 2017

You bring up good points @rmmmp, and you are correct I was not implying porting all of your excellent work into emberfire, merely bringing it up as a topic of discussion, which we are having now :)

As for the target release date, I don't mean anything negative towards the ember/data team but the delay of the latest release, combined with the sheer amount of bugs in it, makes me think 2.15 is a bit further from aug 14th.

Additionally, I like the idea about piecemeal-updates, but the firebase dev team here has not responded or merged PRs or even made a code change in almost 2 months, so my thought process was -> refactor a bunch of stuff all at once and then... i dunno.. appeal for the PR to be brought in? otherwise just keep it a hard fork and carry on if firebase is not interested in maintaining this repo.

I am conflicted either way, as I want to keep the work here, but at the same time piecemeal PRs will do no good if we don't have a responsive dev team to review and merge them consistently.

In other words, I have no idea, but I'm still grooming the list of work to be done so once I can get that up it will be more clear (hopefully) how to proceed.

@mileszim mileszim mentioned this issue Aug 2, 2017
@RobbieTheWagner
Copy link

@mileszim perhaps we should just create a new addon for integrating with firebase and use some of the work from here and improve upon it? I agree it would be nice to have the official version updated and working, but something with this many moving parts cannot just sit idle for months with no updates.

@elgordino
Copy link

Thanks for getting this discussion going @mileszim . I would love to see a ember firebase integration that was quicker to evolve and more future focused. I think the ideal would be for this to happen within the current project, and I think we should allow for the opportunity for that to happen, however it appears that it will be unsuccessful then a fork/rewrite would be the only option really.

The priority features that I think emberfire is missing right now:

  • Atomically committing relationships, eg A <-> B, if the relationship is added / removed then it should happen on both A & B in the same update(). This is critical for records that are not parent/child related.
  • Deleting records automatically updating other records with relationships into that record.
  • Transaction Support. Initially covering relationship updates in the first two items. In the longer term I would like to be able to something like .save(transactionID) on multiple records and then later commit(transactionId).

I have a gist that I put together to achieve some of this because I need to for my current project. I'm certainly not suggesting it as a proposed implementation, but I would like to see something that provided this functionality.

  • Models to be able to express their own path prefixes (determined at runtime), or for id's to contain paths. There has been discussion on this topic in [Proposal] Allow paths in ids #432 and @rmmmp has done some work already in emberfire-utils. I am currently using a different, perhaps orthogonal, approach to the same requirement detailed in this gist.

  • A simple way to automatically set the server timestamp each time a record is saved, e.g. for a lastModified field. gist.

Some other ideas

  • Drop support for everything but the the latest LTS and the one prior? Or even just latest LTS?
  • Feature flags for new features, this will help ensure we stay in sync around a single branch as more complex/experimental features are delivered

@mileszim
Copy link
Author

mileszim commented Aug 3, 2017

This is great @elgordino, thanks for taking the time to write all this out.

The three bullets at the top, I'll weight in my opinion: I'm, preferential (but not stubbornly dedicated) for having a transaction being a function wrapper instead of a separate entity, but that depends on the firebase js API. Something like this:

this.get('store').transaction((store) => {
  const record = store.createRecord(...);
  ...
  record.save();
});

Atomic relationship saving in firebase, if I remember, are harder (or used to be not possible)? I just woke up so I'm could be totally wrong, but I'd love to see that and what you've tried or successfully implemented! Once we have the list (soon i promise :)) this could be delegated to you @elgordino?

As for deleting the relationship to the a record that's deleted, that will be a fun ride down internal model map and emberfire's legacy code. It should be implemented correctly in ember/data 2.15 from the code I've read so our work should in theory be based around correctly handling it on the emberfire side.

The middle two features regarding path prefixes and lastModified, you've done great work getting the ball rolling so I think that will be a straightforward one :)

For the last bit, as far as I know emberfire as it stands now works with 2.12 (or at least I used it a lot just fine), its 2.13 and up that introduced issues. If community consensus wants that, lets do it, and continue these kinds of discussions as we update piecemeal.

How does this all sound?

@jamesdaniels
Copy link
Contributor

Just FYI I've spoken to my manager and will be setting aside some time this quarter to give some major love to the project; including dealing with the legacy code. I have some angularfire2 commitments on my plate this month though, so I probably won't start doing any heavy lifting until September.

Let's keep the conversation going on what we want this library to look like + I'd welcome help from anyone in the community 😄

@Cryrivers
Copy link
Contributor

Résumé submitted. (jking).

@jamesdaniels jamesdaniels self-assigned this Aug 4, 2017
@jamesdaniels
Copy link
Contributor

Just floating it but it would be much much much easier for us on the Firebase team to maintain this project it we slimmed it down. Wrapping the entirety of the Firebase JS SDK, which has dedicated staff and is constantly fixing bugs + releasing new features, is untenable IMO.

The framework-specific libraries are maintained as side-projects by those of us interested. Then of course as we get settled in at Google we no longer use Ember in our day-job and our interest / knowledge of the state-of-the-art wanes.

Emberfire should be the very base driver, allowing Firebase to work with Ember with the minimal amount of fuss; rather than completely abstracting Firebase away. We've taken this approach with angularfire2 which I've been doing some work on lately and I've found it refreshing.

If the community wanted to completely wrap and abstract away Firebase making it a 100% drop in replacement for a traditional backend+RDMS w/transactions, which Ember-data was really made for, then they could build on top... but it would be never ending pile of work.

@mikkopaderes
Copy link
Contributor

@jamesdaniels, are you implying that emberfire should just expose a firebase service then adapters, sessions, etc. would be dependent on that service and each being a community maintained addon outside of this? If so, I'm in favor of that though it would be nice if the separate addons were also maintained officially. However, I can understand the reasoning why it would be hard for those to be officially maintained.

@elgordino
Copy link

@mileszim re atomic relationship changes. It is possible if you make an update to both sides of the relationship in the same update() call.

docs

Using these paths, you can perform simultaneous updates to multiple locations in the JSON tree with a single call to update(), such as how this example creates the new post in both locations. Simultaneous updates made this way are atomic: either all updates succeed or all updates fail.

Re deleting records and updating relationships (at least those with an two-way relationship), this is actually easier than it might sound, you don't need to retrieve the related model in Ember Data you can just set the data in Firebase at the location where its relationship would be defined to null.

eg if foo-belongsTo-bar and bar-hasMany-foo and you want to delete foo you can, in an update() do this
{ '/foos/<foo.id>' = null, '/bars/<bar.id>/foos/<foo.id>' = null}

If bar is already loaded then it will be updated automatically due to it observing the data being changed. I'm using this technique in the gist I linked and it's working fine.

@RobbieTheWagner
Copy link

@jamesdaniels if I am understanding you correctly, I disagree with that idea. The beauty of Emberfire is that any Ember developer can essentially just set up their models correctly in Ember data and all the firebase stuff is handled for them behind the scenes. It allows for rapid prototyping and for anyone to quickly spin up a database and not have to worry about how it works.

@mikkopaderes
Copy link
Contributor

@jamesdaniels what are your plans for firestore? 😉

@jamesdaniels
Copy link
Contributor

@rmmmp as mentioned on #529, Cloud Firestore support will be coming as a WIP soon. This will become our preferred adapter as it much better aligns with the needs of ember-data than the Firebase Realtime Database. Cross collection transactions, shallow fetches, etc.

Knowing Cloud FIrestore was coming was also my reasoning behind tempering expectations with the current ember-data adapter.

I will release a roadmap shortly. I am hoping to roughly align the new Cloud Firestore based adapter with the Ember 3.0 schedule.

@jamesdaniels
Copy link
Contributor

@rmmmp happy to work with you and the rest of the community FYI + not do this in a vacuum.

@RobbieTheWagner
Copy link

@jamesdaniels happy to help where I can, just need some direction on what needs to be done 😄

@jabbermarky
Copy link

@jamesdaniels - by your silence have you effectively moved on to other things or is this project still alive? Need to know so that I can adjust my use of Firebase accordingly...

@jamesdaniels
Copy link
Contributor

The project is still alive. A WIP on the next major version of the plugin is in play but I got a bunch of other stuff put on my plate at the end of 2017.

I'm just getting back from vacation and will carve out some time to put a bow on it soon.

In the meantime I'd suggest using Firestore with the Firebase JS SDK without an Ember-data adapter. The JS SDK is Promise based so it plays nicely with Ember out of the box + it's good to develop an understanding of your underlying data-store, even if we end up hiding it away under an adapter later.

@jamesdaniels
Copy link
Contributor

I'm still aiming to roughly align with Ember 3 btw ;) I'd preferred to have cut a first RC before Xmas but never enough time in December.

@jabbermarky
Copy link

Thanks for the update @jamesdaniels . I personally can't afford to revise my architecture to go directly at Firestore using the Firebase JS SDK. If that is my best option, then I need to reevaluate my Firebase decision. If on the other hand there is something coming with Ember 3, then I can wait and see.

@RobbieTheWagner
Copy link

I agree that this project is vital to using with ember. If we lose the ability to interface with Firebase via ember-data, we lose a lot of the goodness of working with ember and using the vanilla JS SDK does not align with the ember way of doing things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants