Skip to content

Blockers migrating from SDK 1.5.x -> 1.6.x #73

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

Closed
TylerBrock opened this issue Oct 29, 2015 · 27 comments
Closed

Blockers migrating from SDK 1.5.x -> 1.6.x #73

TylerBrock opened this issue Oct 29, 2015 · 27 comments

Comments

@TylerBrock
Copy link
Contributor

I understand that the JS SDK is moving away from backbone style objects but there are a couple of pieces of that functionality that we'd like to see make their way into the 1.6 SDK at some point.

  • Our application depends on the ability to silently update attributes of Parse.Objects

    Our existing parse objects (called models in our application's vernacular) all override validate() to provide client and server side validation for objects. We validate them before they are sent to Parse in the browser and after Parse receives them via a beforeSave hook. While it is not always the case, we often depend on being able to have objects in a non-valid state up until the point they are saved by constructing and updating them with {silent: true}.

    var thing = new Models.Thing({"awesome": true}), {silent: true});
    thing.set("awesome", true, {silent: true});

    The reason we do this is because we want to build up a model with information we receive over time. For example a user wants to create a group in our system and we know the organizationId that it belongs to up front but don't have the other details such as group name and location.

    You can imagine having a component in React that renders a form with data that you currently have about an object (that can't be saved as-is) but that can have the missing pieces filled in by the user to make it ultimately validate cleanly.

    The API doesn't have to be the same but it would be great if you could construct objects and manipulate their attributes freely UNTIL you try and save it, at which point everything must be valid. This surfaces as thousands of "Uncaught Error: Can't create an invalid Parse Object" in our application.

  • Our application depends on defaults

    This one is easier to work around but as a result of the way we use validation (described in the first bullet point) we depend on some values having defaults.

Here is an example of a HustleModel and HustleModel's implementation where you can see defaults and constraints in use. I don't mind that this code is publicly visible.

How can we help add this functionality or work around is absence in versions 1.6+ of the SDK? I'd like to help in any way possible so that we can stay current with bugfixes, etc (the retrying of API requests in the new RESTController especially) and use more ES6.

@andrewimm
Copy link
Contributor

Defaults sounds like an easy enough thing to add; we support legacy style initialize() methods, so handling defaults should not be too different.

I definitely understand the need for explicitly avoiding validation; if anything, let's just enable it and call it something else. "Silent," to me, refers to Backbone's "change" event; simply passing { ignoreValidation: true } as the options property.

Feel free to work on implementing these yourself, I'm happy to look at PRs as long as you provide unit tests for functionality. Otherwise, I can try to get to some of these within a week.

@TylerBrock
Copy link
Contributor Author

Ok, thank you.

As a thought experiment would it not be better to have validation work that way by default?

It's quite strict to enforce that the object be valid at all times. I feel as though most users do not want validation on every call to set() or during construction. They can always call validate explicitly if they want to check it before attempting to persist it.

As an example of this being common practice see ActiveModel of rails fame or Django's ORM validation.

@andrewimm
Copy link
Contributor

I think I agree with that. We were simply replicating the behavior from <1.6.
Validate on save seems more sane than validate on set

@typhonrt
Copy link

If you are looking for Backbone like validation perhaps checkout Backbone-Parse-ES6 which incorporates the latest Backbone 1.2.3 + Parse 1.6.8 APIs at the moment.

https://github.com/typhonjs-parse/backbone-parse-es6

Just grab an ES5 bundle of your choosing from /dist directory. You'll need to rename your extends to Backbone.Model of course, but the old Parse.Object extends signature is supported. The ability to do validation and silent setting should be captured in Backbone-Parse-ES6 as that has more to do with Backbone. Validation as a control flow mechanism mentioned definitively was coming from the forked Backbone API which Parse previously relied on.

I'll be porting over Backbone 1.2.4 as soon as it's released and a bunch of tests from the Backbone repos and create new Parse tests.

Andrew... Want to get a blog post up about Backbone-Parse-ES6 next week? I'm finishing a comprehensive demo repo this weekend + lots of tutorial documentation which rounds out complete support for Backbone continuation w/ Parse. Drop me an email at [email protected]

@TylerBrock
Copy link
Contributor Author

Another blocker we seem to have is that our font end and back end share our models.

The front end needs require('parse') and the back end needs require('parse/node') but neither do the trick. When we use parse in the models it thinks parse is not initialized on the front end. When we use parse/node it complains on that there isn't any localStorage because we aren't inside of a browser.

Perhaps we can write our own shim that requires the right thing depending on whether we are in a browser or not but it seems like I should just be able to require('parse') and let the library sort itself out.

@andrewimm
Copy link
Contributor

We have to do compile-time branching of parse vs parse/node, because we use ES6 imports that are deterministic. This is ideal for developers -- you wouldn't want imports to be determined at runtime, because this is essentially equivalent to solving the halting problem.
Client vs Server behavior also branches on more than just localStorage and patching XMLHttpRequest. The server version disables the concept of Current User and does not enable single-instance object mode, since these concepts are unsafe in a shared-memory environment. In general, you wouldn't want to ship the same SDK codebase to both systems, let alone ship the same behavior.

What kind of build system are you using for your app? It shouldn't be too much effort to put together a shim like the one we use in Parse+React

@TylerBrock
Copy link
Contributor Author

We are using gulp + webpack on the front end. I'm familiar with the reasons to separate the behavior, I'm the one who reported the security problem with the original SDK 😃

@TylerBrock
Copy link
Contributor Author

Here's another one, this code used to work:

var goalPromise = goalStep.get("goal").fetch();
goalPromise.then(function() { ... })

Now we get:

TypeError: goalStep.get(...).fetch is not a function

goalStep.get("goal") returns a pointer object to an instance of the Goal class, shouldn't the SDK hydrate that into a Parse.Object subclass that has the fetch() method?

@andrewimm
Copy link
Contributor

That depends on how goalStep was constructed. This is one of those cases where I need more code to be able to effectively debug anything.

@TylerBrock
Copy link
Contributor Author

The goal in goalStep looks like this:

{__type: "Pointer", className: "Goal", objectId: "lmgbtrKX4Z"}

On saving we get: invalid type for key goal, expected *Goal, but got map

@andrewimm
Copy link
Contributor

You're right that it should be inflated, and should not be a map with the over-the-wire format. Still, I don't know how goalStep was created, so I can't comment on it

@TylerBrock
Copy link
Contributor Author

I think what i boils down to (at least in some cases) is that setting an Parse.Object or subclass instance on a pointer field now requires that we call toPointer() instead of automatically converting it to a pointer for some reason. Is that intended?

It seems to me that the old behavior is better:

  • have object with pointer -> instance field
  • have instance
  • set objects pointer field to instance
  • instance converted to pointer implicitly upon save()

We have a problem in which we sync state between components/objects in React and can't call toPointer() because an unsaved parse object cannot be serialized so we depend on the conversion happening just in time before saving in a parent/child relationship.

Specifically we have a goal with many goal steps. They are linked as react components because when we create the steps inside of the goal they are passed the goal as a prop so they can point to the parent goal. When we go to save the goal + goal steps we save the goal first, then the steps. The problem being that when the steps are saved we can finally convert goal to a pointer but not beforehand.

@andrewimm
Copy link
Contributor

It works the same way it always has:

let a = new Parse.Object('Item');
let b = new Parse.Object('Item');
a.save({ child: b }).then(() => {
  // Item's "child" field is now a Pointer to _Item

  b.save('child', new Parse.Object('Item')); // this works fine
});

If you have a new instance of the app, and you just fetch a without any includes, it'll be an object with an empty (attribute-less) Parse Object on the child field, exactly as you describe the legacy behavior. Pointers are only an over-the-wire and database expression tool. As far as the SDK is concerned, once it has inflated objects coming from the server, Pointers do not exist.

Once again, because I have zero context on what goalStep is (is it a newly-created object? is it the result of a query? did it come from cloud code?), and zero context on what the rest of the surrounding code looks like, I can't comment on your particular issue.

@TylerBrock
Copy link
Contributor Author

Both Goals and GoalSteps are completely new objects.

// goals require group pointer
var group;
var query = new Parse.Query(Group);
query.first().then(function(g){
  group = g;
});

// the setup
var goal = new Goal({name: "Great Goal", group: group})
var goalStep = new GoalStep({"goal": goal});

goal.save() // works fine
goalStep.save().fail(function(error){
  console.log(error)
})

Output:

ParseError {
  code: 111,
  message: 'invalid type for key goal, expected *Goal, but got map'
}

@TylerBrock
Copy link
Contributor Author

Perhaps you could explain how the rest api differentiates between a Goal* and an object of the following format:

{goal: {__type: "Pointer", className: "Goal", objectId: "Hk4NM8RBvg"},}

I see a bunch of requests in a batch save goals where that format works just fine and then others where it does not work and I get the error message 111. I'm perplexed to say the least.

@TylerBrock
Copy link
Contributor Author

This is what is being sent to the server by chrome, i did some formatting so it might be slightly off but basically we see some go through and others that don't:

{
  "requests":[{
      "method":"POST",
      "body": {
        "goal": {
           "__type":"Pointer",
          "className":"Goal",
          "objectId":"Hk4NM8RBvg"
         },
         "organization": {
           "id":{"className":"Organization","objectId":"ED36Rz8T5Z"},
           "name":"xxx"
         },
         "type":"invite",
         "eligibilityWindowRelativeStart":-604800,
         "eligibilityWindowRelativeEnd":-172800,
         "agentReminderMessage":"Time to start sending invitations for q",
         "targetingSpec": {
           "__type":"Pointer",
           "className":"TargetingSpec",
           "objectId":"9HKc0sTIpU"
        }
      },
      "path":"/1/classes/GoalStep"
  ]},
  "_ApplicationId":"",
  "_JavaScriptKey":"",
  "_ClientVersion":"js1.6.8",
  "_InstallationId":"",
  "_SessionToken":""
}

@andrewimm
Copy link
Contributor

Hmm, if it's as you say, that might be a backend issue. That should be a totally valid POST body.

@TylerBrock
Copy link
Contributor Author

I'll file a bug

@TylerBrock
Copy link
Contributor Author

Ok that one is filed, I posted it with the exact curl request that Chrome is making so that you guys can try for your selves. It's generated from the SDK and I think I'm doing everything correctly so let me know if that is not the case.

Here is another common pattern we used to use where we had an id but no object yet:

const thing = new Thing({id: props.selectedGroupId});
const query = (new Parse.Query(OtherThang)).equalTo('thing', thing);

Is the proper way to transform this query to do the following?

const group = Thing.createWithoutData(props.selectedGroupId).toPointer();

@TylerBrock
Copy link
Contributor Author

Also the legacy initializers in our codebase need refactoring to serve as defaults, the documentation states:

//You should call either:
    var MyClass = Parse.Object.extend("MyClass", {
        Instance methods,
        initialize: function(attrs, options) {
            this.someInstanceProperty = [],
            Other instance properties
        }
    }, {
        Class properties
    });
// or, for Backbone compatibility:
    var MyClass = Parse.Object.extend({
        className: "MyClass",
        Instance methods,
        initialize: function(attrs, options) {
            this.someInstanceProperty = [],
            Other instance properties
        }
    }, {
        Class properties
    });

However neither of those initializer functions would work today with 1.6.x SDK because initialize is implemented like this:

// Enable legacy initializers
if (typeof this.initialize === 'function') {
  this.initialize.apply(this, arguments);
}

This would make the previous examples just set properties of the Parse.Object subclass but they don't wind up in attributes. This means those properties do not go through validation and I believe this means they are not persisted.

@TylerBrock
Copy link
Contributor Author

Hey @andrewimm the problem saving our GoalSteps wound up being an issue with our beforeSave() hook implementation which was being tunneled back through to the original save so we can scratch that one off the list!

@andrewimm
Copy link
Contributor

That's why I enjoy reproducible test cases :)

@andrewimm
Copy link
Contributor

Re: initialize, it does exactly what it is intended to do. The example you cite may not be the best, but that's only because it's a near-direct copy of Backbone's documentation. It's used for setting values on the instance, not for setting attributes; if you wanted to establish defaults for attributes, you'd have to set() them in the initializer.

@TylerBrock
Copy link
Contributor Author

Ok, cool that is what we wound up doing. The only reason we tried that is because I think you suggested that as a possible solution for the lack of a defaults feature above. Would you accept a pull request if I were to implement defaults? It's no longer a blocker for us but it would be nice to have right?

@TylerBrock
Copy link
Contributor Author

TylerBrock commented Nov 10, 2015

That being said, the only real blocker we have left which has us scratching our head is #89. Would appreciate it very much if you could take a look and let us know if we should not longer expect that to work or if that is indeed a bug and we can tackle it together. :-)

@andrewimm
Copy link
Contributor

I'd definitely accept a PR for defaults, but it might involve some API discussion first.

The array of pointers thing in #89 is interesting, but I think the fix might just involve tweaking a boolean. It reminds me of another issue I've been trying to track down.

@simonaberry
Copy link

iniitialize issue (see #322) fixed by 03d14a0

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

4 participants