clintjhill / ember-parse-adapter

An Ember Data Adapter for Parse
http://clintjhill.github.com/ember-parse-adapter
MIT License
190 stars 53 forks source link

Collaboration: ember-parse <> ember-parse-adapter #69

Closed gcollazo closed 9 years ago

gcollazo commented 9 years ago

Hi @clintjhill,

I just uploaded a fork of this repo to https://github.com/GetBlimp/ember-parse. One of the main differences is that I'm using the JavaScript Key instead of the REST API Key.

My decision not to use the REST API Key was based on a conversation I had with a friend who works at Parse. He told me that as a general rule the REST API Key should be kept secret. He cited a few reasons for that but I only remember one: "turning off client side push does not affect the REST API Key".

I also had a few problems with relationships with the current adapter and needed a way to do authentication. For auth I wrote a service which is injected to routes, controllers and components and provides methods to authenticate, invalidate, sign up and request password reset. This session service is implemented without any external dependencies.

For my adapter and serializer I basically copied the code from this repo and fixed the issues I came across with. As of right now I think almost everything is working (for my use cases). I have to admit that to solve some issues I had to hack my way through. I know it can be better.

I would like to propose a collaboration between both maintainers to make this addon better for all. I'm open to feedback or any suggestions. I would love to work together to build a shared solution.

:smile: What do you say?

oriSomething commented 9 years ago

do you have any link to approve the rest key stuff in the docs? sounds wired, because in the docs they say There are many things you can do with the REST API. For example: A mobile website can access Parse data from Javascript.

hramos commented 9 years ago

Hey all, Parser here. I advised @gcollazo against using the REST API Key for this use case. You can find this in the Parse REST API docs, under "Calling from Client Apps", and I quote:

You should not use the REST API Key in client apps (i.e. code you distribute to your customers). If the Parse SDK is available for your client platform, we recommend using our SDK instead of the REST API. If you must call the REST API directly from the client, you should use the corresponding client-side Parse key for that plaform (e.g. JavaScript Key, Client Key for iOS/Android, or Windows Key). If there is no Parse SDK for your client platform, please use your app's Client Key to call the REST API. Requests made with the Client Key, JavaScript Key, or Windows Key are restricted by client-side app settings that you configure in your Parse.com app dashboard. These settings make your app more secure. For example, turning off the "Client Push Enabled" setting will prevent push notifications from being sent from any device using the Client Key, JavaScript Key, or Windows Key (recommended for all production apps). Therefore, you should not distribute any app code with the REST API key embedded in it.

In summary, the REST API Key is meant to be kept private. Only the Client Key, JavaScript Key, and Windows Key can be safely exposed in a client app.

gcollazo commented 9 years ago

@oriSomething take a look at what @hramos wrote. Thanks Héctor.

joshfester commented 9 years ago

+1 for moving to a repo with a more active owner

mixonic commented 9 years ago

@gcollazo I have commit on this repo. @clintjhill is still the only one with publishing.

I strongly believe we should find a way to keep effort on a parse adapter in one project. Parse's API is fairly difficult to wrap in Ember-Data due to widely differing assumptions about what an API should look like. For example, Parse does not return a full set of properties after an update. Ember-Data presumes those missing properties are null.

I would love, love to see more help on this codebase. I fought my way in to try and help others. To move forward, we must focus on tested patches so we can build on top of previous work.

Commits like this one with the comment "batshitcrazy" don't provide a foundation to build upon. It completely disregards a legitimate problem with Parse's API.

It makes me very sad to see so many potential contributors saying "I forked, works for me", and dropping mic to walk away. What can I do to help you land changes upstream?

oriSomething commented 9 years ago

@mixonic i think the reason people react as they do, because sometimes, it seems that this repo is abandoned. until recently you couldn't use it easily with ember-cli app and even now, when you enter to the suppose npm page, it's not there. for some it might seem like a small thing, but it prevent people to take this project seriously.

mixonic commented 9 years ago

I'm somewhat intimidated by the changes @notmessenger made, as they were largely unreviewable. @clintjhill merged them in and I'd love to see them released if he can do that.

@clintjhill can you add me to the npm repo?

@gcollazo if you are interested in being a more active maintainer I would love to help you get changes upstream. I am very happy to code review and work with you until we can make you a maintainer.

mixonic commented 9 years ago

until recently you couldn't use it easily with ember-cli app

FWIW, unfortunately the ember-cli changes also removed the ability to use the adapter without ember-cli. So that was more of a lateral move pleasing new users than building a solid base for existing users. Exactly the kind of thing you would not like to see happen again, I presume.

joshfester commented 9 years ago

@mixonic I am guilty of dropping the mic. My reason is all the hurdles involved in moving from my custom fork back to this one. Until recently this repo was completely abandoned, so my company took the path of least resistance. Now we're stuck until this repo has more features and becomes stable.

I'd love to help out in any way I can. I'm currently helping @notmessenger with a pull request.

I don't feel qualified to work on the features my company needs until there's more of a discussion on how they should be implemented (user roles, cloud functions, ACLs).

oriSomething commented 9 years ago

@mixonic true, but i think, especially you as a core contributor to ember, you know where is the future of ember going, and it's question of time until pretty much all ember project will have to move to ember cli for the reason it might be and only exceptional project won't use the ember-cli. so yes, but it's a necessary painful step in this repo.

clintjhill commented 9 years ago

Friends, first - my apologies. I feel pretty guilty for what I'm reading here. Honestly I had no idea there was so much desire behind this codebase, and it's because I did a poor job of paying attention. I have excuses like a day job etc, but none for the lack of attention.

I'm happy to provide access to whoever needs it. @mixonic I'm not aware of an npm repo. I believe that's @gcollazo that has one.

When I started this repo it was really academic for me because I felt like Ember Data was a winner. I also thought Parse was a winner. Still feel that way. So it makes me super excited to have you all help with this.

But regardless, please direct me to however I can help.

mixonic commented 9 years ago

Whoa. There is no npm package! News to me. I will make one.

mixonic commented 9 years ago

I've published the package here: https://www.npmjs.com/package/ember-parse-adapter and added @clintjhill as an owner.

gcollazo commented 9 years ago

@mixonic I totally agree with "keep effort on a parse adapter in one project". That's why I opened this issue in an effort to try to understand what it would be required to join forces.

I started work on this by forking the project with a future PR in mind because "this should be easy". But the changes where so substancial that I thought it would be better to get the project in the best shape I could following my vision to later ask you guys if you liked the direction I was going with the project. This is me asking you guys.

I made substancial changes to the project and believe that if I wanted to convince you guys of what I was thinking the direction of project should be I had to show up here with actual working code. "Code wins arguments" as they say.

What can I do to help you land changes upstream?

I would love to get a review on my fork and a comment about my approach of having an adapter, serializer and session service on the same addon. My goal is to build a single Parse addon that wraps all of their services. If you guys agree with that approach and you think my code is not complete garbage I would love get some direction on what should be the next step. I would love to help.

"I forked, works for me", and dropping mic to walk away

If this is the perception of my attitude let me assure that's not my intention. I came here with the most sincere desire of collaboration. I already have something that works for my immediate needs but thought more people in the community might benefit from my work. Let's work together.

/me picks up the mic :stuck_out_tongue_winking_eye: :hand: :microphone:

quantuminformation commented 9 years ago

Great to see all this discussion tonight. Great to hear from the original author too. I look forward to getting this project the more usable state so I can use it for my social network.

Kind regards Nikos

mixonic commented 9 years ago

Clint enabled Travis-CI and we have a badge on the readme now. This should make it easier to sanity-check PRs.

joshfester commented 9 years ago

@gcollazo I like the idea of having all Parse services in one place, but I'm not sure about the session. What was your reason for going that route rather than using Ember Simple Auth?

oriSomething commented 9 years ago

@joshfester the user objects of Parse are quite binded to their auth system. you also get the current user data when you login. so it's kind of make sense that it will be in this project

oriSomething commented 9 years ago

you can reopen the class?

notmessenger commented 9 years ago

I too am interested in finding a way to help out with this project and to move its progress forward. I am not using it for my employer but am very interested in using it for a personal project I am working on.

I do not know if it is the desired approach or not (@mixonic and I are having a conversation about it on my latest PR - https://github.com/clintjhill/ember-parse-adapter/pull/70) but I have been going over @joshfester's branch and extracting commits that apply to the version of Ember Data one increment greater than the current dependency of this repo. I will then continue to do so, walking this repo up through the Ember CLI and Ember Data version releases, until caught up with the current releases of both. My thinking is that at this point it buys us having all of the separate development efforts (at least the ones I am aware of) consolidated into this single one and us being against the latest version of the libraries. Then at that point we can begin refactoring and adding new features.

I'm open to approaching this a different way as well but regardless of the approach am in agreement that development effort towards a single project would be beneficial.

An idea that just came to mind while I was typing this is possibly adopting a/the RFC pattern of https://github.com/ember-cli/rfcs and https://github.com/emberjs/rfcs where we propose and discuss new features and possible implementations of how to do so, our understanding of Parse integration, etc. Then once these have been vetted they can be flagged for development, at which point someone (or several someones) can begin doing so. This would help organize development efforts as well as provide visibility into where things are heading.

joshfester commented 9 years ago

I love the idea of following the RFC pattern. Once we get this repo up to date, it will be difficult to move forward if there's no standard way to introduce new features.

notmessenger commented 9 years ago

@gcollazo @mixonic I removed the support for the Javascript key because it was not being used in either the code, tests or demo when I made the conversion to Ember CLI. I can most certainly make a PR to put it back. Based on the comments by @hramos and the corresponding conversation, do I remove the REST API key?

notmessenger commented 9 years ago

One of the things we definitely need are more robust tests. I noticed while working on https://github.com/clintjhill/ember-parse-adapter/pull/70 that the tests still passed even though the demo didn't work and after adding https://github.com/clintjhill/ember-parse-adapter/pull/70/files#diff-4cd795786804c68db2986e055d96d561R155 (with @joshfester's assistance) the tests continued to pass as well.

Again, my approach thus far has been to get everything pulled together, using the existing tests and demo as my litmus test, knowing that they are lacking but seem to be what has been acceptable thus far. I then intend to circle back around and begin refactoring and testing lots of things.

quantuminformation commented 9 years ago

Dear all, I've had a look at this fork and there really is some powerful stuff going on in regards to the ACL's + I like having all the auth stuff native to the addon. I've used simple auth too but this fork has a nice simple implementation. I'm going to try port my social network over to the fork and see how it goes and will report back..

jacobthemyth commented 9 years ago

What is the plan/status on an RFC process? I would love to be able to start planning how to move forward with the library.

jacobthemyth commented 9 years ago

It seems like this issue has become a discussion without a goal, which means it might be open perpetually. I just split out some relevant issues, are there any other outstanding questions that should be brought into more focused issues? If not, I propose this issue be closed, and any open questions be translated into RFC issues.

mixonic commented 9 years ago

agreed, closing.