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

1.9 #272

Closed
wants to merge 44 commits into from
Closed

1.9 #272

wants to merge 44 commits into from

Conversation

ritch
Copy link
Member

@ritch ritch commented May 21, 2014

/to @raymondfeng @bajtos @crandmck

Breaking Changes

See https://github.com/strongloop/loopback/blob/1.9/CHANGES.md for my notes on breaking changes.

Should we mark these in the docs as deprecated?

Model => DataModel

Model is now specifically for defining data structures. DataModel is the default class for app.model(name, ...); and models.json.

Remote Methods

Remote methods no longer rely on function objects for storing meta-data. This means remoting meta-data is simple to (de)serialize. All metadata has been moved from juggler to loopback including the relations.

Types

Remote methods now define the concrete type for accepts / returns. This is required to correctly serialize models when code expects the concrete type (eg. MyModel) in a callback. This is to allow code like the following to work anywhere (client, server, browser, etc)

MyModel.find(function(err, myModel) {
  myModel.name = 'foobar';
  myModel.save();
});

Replication

This has been mostly reviewed as part of #153. End to end tests should be review and glaring issues preventing releasing 1.9.0. Minor issues can be resolved in 1.9.x.

The e2e tests could be improved quite a bit. Suggestions welcome there.

Remote Connector

This has been reworked a little, could use some second eyes. Same story here, anything major should be fixed before we release 1.9.0. Minor changes can be saved 1.9.x.

Isomorphic LoopBack

This feature was landed a while ago, overall testing strategy could use some eyes. Changes suggested here will most likely wait for 1.9.1.

ritch added 14 commits May 15, 2014 17:31
creating a cache
 - Use the SharedClass class to build the remote connector
 - Change default base model from Model to DataModel
 - Fix DataModel errors not logging correct method names
 - Use the strong-remoting 1.4 resolver API to resolve dynamic remote
methods (relation api)
 - Remove use of fn object for storing remoting meta data
Allow juggler to mix in these methods.
@slnode
Copy link

slnode commented May 21, 2014

Test Failed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/1282/

@slnode
Copy link

slnode commented May 21, 2014

Test Failed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/1283/

console.log(step.name);
step();
}
}, SPEED);
Copy link
Member

Choose a reason for hiding this comment

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

The sample app hangs after the last step was executed.

I'd say the interval should be cleared when there are no more steps to run.

@bajtos
Copy link
Member

bajtos commented May 22, 2014

I am worried about the implications of the change from Model to DataModel on backwards compatibility.

The fact that you are changing base classes of built-in models like User indicates this change is breaking backwards compatibility, as all existing loopback applications calling Model.extend have to be changed to DataModel.extend. Please correct me if I am wrong.

It took me some time to figure out what is the difference between Model and DataModel and I already know a lot about LoopBack internals. Regular loopback users won't be able to figure this out themselves.

The document Changes.md should explain why this change was made, what is the difference between Model and DataModel, when to use which one and most importantly, how this change affects existing application and what steps should be made to fix possible incompatibilities.

@bajtos
Copy link
Member

bajtos commented May 22, 2014

One more thing: I find it rather confusing that loopback.createModel is using DataModel instead of Model as the base class.

It is also changing the behaviour of existing apps, consider the following example:

// mqtt is a connector sending messages to an IoT device
var mqtt = loopback.createDataSource('mqtt', { connector: 'mqtt', /* etc. */});
// Message is representing the payload to send. It does not have DAO methods.
var Message = loopback.createModel('message', /* etc. */);
Message.attachTo(mqtt);

Here is my proposal:

  • Preserve 100% backwards compatibility. Keep loopback.createModel the same as it is. Make sure that when a Model is attached to a datasource, CRUD operations works correctly.
  • To enable advanced features like sync, ask developers to change their model definition and set DataModel as the base class where appropriate.

If that's not possible, then I am afraid this change should be released as loopback 2.x.

@bajtos
Copy link
Member

bajtos commented May 22, 2014

Another idea: for models created via models.json, we can detect what datasource they are being attached to and use the appropriate base class (DataModel for database-like datasources, Model for other kinds).

@bajtos
Copy link
Member

bajtos commented May 22, 2014

The name DataModel does not capture the distinction from Model very well. Can we come up with a better name, e.g. CrudModel?

* **Options**
*
* - `trackChanges` - If true, changes to the model will be tracked. **Required
* for replication.**
Copy link
Member

Choose a reason for hiding this comment

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

Is trackChanges used by Model at all? Isn't it an option of DataModel only?

@ritch
Copy link
Member Author

ritch commented May 23, 2014

Closed in favor of: #276

@ritch ritch closed this May 23, 2014
@ritch ritch mentioned this pull request May 23, 2014
Merged
33 tasks
@bajtos bajtos mentioned this pull request Jul 22, 2014
47 tasks
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

Successfully merging this pull request may close these issues.

4 participants