jagi / meteor-astronomy

Model layer for Meteor
https://atmospherejs.com/jagi/astronomy
MIT License
605 stars 66 forks source link

Astronomy 2.0 #236

Closed lukejagodzinski closed 8 years ago

lukejagodzinski commented 8 years ago

I've started this issue to share progress on Astronomy 2.0. I wanted to release RC version before the end of this year but it will be published before the end of Q1 in 2016. I've rewritten almost entire package to make it faster (23 times faster document initialisation), less error prone and full-featured.

CURRENT VERSIONS:

NEW FEATURES:

There are many new minor features that I will be adding to the list. There are also some changes to API about which I will be writing more soon. Astronomy 2.0 will also fix many bugs or problems from 1.0.

MAIN CHANGES IN V2 COMPARING TO V1:

// v1
fields: {
  firstName: 'string'
}
// v2
fields: {
  firstName: String
}
// v1
fields: {
    address: {
        type: 'object',
        nested: 'Address'
    },
    phones: {
        type: 'array',
        nested: 'Phone'
    }
}
// v2
fields: {
  address: Address,
  phones: [Phones]
}
events: {
  beforeSave: function(e) {
    let doc = e.target();
  }
}
var user = new User();
// v1
u.set('firstName', 'John');
// v2
u.firstName = 'John';
// however you can still write for nested fields
u.set('address.city', 'San Francisco');
fields: {
    firstName: {
        type: String,
        validators: [{ // Single validator with the param
            type: 'minLength',
            param: 5
        }]
    },
    lastName: {
        type: String,
        validators: [{ // Multiple validators with params and custom error message
            type: 'minLength',
            param: 2,
            message: 'Too short'
        }, {
            type: 'maxLength',
            param: 10,
            message: 'Too long'
        }]
    },
    birthDate: {
        type: Date,
        validators: [{ // Validator with the function that resolves to the param
            type: 'lte',
            resolveParam: function() {
                let date = new Date();
                return date.setFullYear(date.getFullYear() - 18);
            }
        }]
    }
}
// client.js file
var user = new User();
user.save(); // It will also save document on the server and trigger all the events and validation on the server
lsunsi commented 8 years ago

@jagi That is AWESOME man. As you well know I'm a heavy user of your package (hence the multiple questions) and I'm super psyched to see what great changes you'll bring to this already great package.

Thanks for all your effort

laurentpayot commented 8 years ago

Amazing. Thanks a lot for all this :+1:

fabiodr commented 8 years ago

Wow! Great!!

ghost commented 8 years ago

@jagi Amazing news !

soaresa commented 8 years ago

That's great!! Thanks a lot!

JulianKingman commented 8 years ago

I'm pretty excited about the relationships, built in methods, and modification stuff, great work!

FYI, Astronomy seems to work fine with react, have you thought about adding that to the docs? It would be interesting to know if there are any differences... it seems like there wouldn't be, since Astronomy really just handles the backend.

ribbedcrown commented 8 years ago

Excellent!!! Great job, thank you!

marbemac commented 8 years ago

Great job, we're quite excited about this :).

Have you considered an immutable option? Working with immutable objects makes it much easier to build performant React apps, because you can take advantage of shouldComponentUpdate with "===" comparisons (fast), instead of deep object comparisons (slow).

https://facebook.github.io/react/docs/advanced-performance.html

Right now:

var p1 = new Post(); // astronomy model called Post
var p2 = p1;
console.log(p1 === p2); // true

p1.set('name', 'different');
console.log(p1 === p2); // true

One example of a project that strikes a nice balance between immutable vs not immutable is https://github.com/Yomguithereal/baobab. Baobab is a state tree, that defaults to immutable, with an option to disable immutability.

It looks like Blaze2 will likely be built on React, and more and more Meteor users are using React, so it's something to consider.

Just food for thought! This is a sore point in our app right now, as it's difficult to optimize our re-renders when working with astronomy objects.

marbemac commented 8 years ago

Another decent immutable reference:

https://github.com/hapijs/joi

lukejagodzinski commented 8 years ago

Thank you all for good words :) I hope you will like changes I'm preparing :)

@JulianKingman it should work with React even better than 1.0, so yes I think that I will add proper information.

@marbemac as using the set method is not required in 2.0 to make changes to the model I think it's a good idea to create module to Astronomy 2.0 that would make astronomy document an immutable record. The only problem is that you have to use mutator methods. By default in Astronomy 2.0 we will have only one mutator method, which is set. However the module for React could provide more methods like push, slice, pop etc. and of course some method that could be used in the shouldComponentUpdate method doc.hasChanged('fieldName') or something like that.

To tell more about React... At the beginning, when I've heard that MDG wants to base future versions of Blaze on React I wan't happy. But after giving React a chance, I think that it's great, it can solve many problems and I will definitely support it in Astronomy. So any advices about features that could make this integration better are welcome :). And as Astronomy is modularised, then it will be very easy to develop some package/module called for example jagi:astronomy-react :)

sergiotapia commented 8 years ago

@jagi Will there be an upgrade path from Astronomy 1 to 2? I'm starting a project and want to know if I should hold off integration Astronomy for now.

lukejagodzinski commented 8 years ago

@sergiotapia 2.0 will not be compatible with 1.0 and projects made in 1.0 will need a few changes to work with 2.0. I will try to write upgrade guide as detailed as possible to make it smooth. If I were you I would wait for 2.0

talha-asad commented 8 years ago
  1. Named behaviors. Add multiple behaviors of the same type to the one class Do you have an example of how this would be defined?
  2. Mapping field name to different field name in collection This sounds like a great feature. Will this mapping be defined in the AstroClass like behaviors?

If you need help implementing these features, i could probably spare sometime. Let me know. Thanks!

lukejagodzinski commented 8 years ago

@talha-asad

  1. I have two ideas for API:
behaviors: {
  customNameA: {
    type: 'timestamp',
    options: {} /* optional */
  },
  customNameB: {
    type: 'timestamp',
    options: {} /* optional */
  }
}
behaviors: {
  timestamp: {
    /* optional options */
  },
  slug: [
    { /* optional options */ }, /* first behavior */
    { /* optional options */ } /* second behavior */
  ]
}

However, I will probably use the first approach. Because it will be easier to manage behaviors by name than by index and name.

  1. Yes mapping will be defined next to the field definition.

Any help would be appreciated. However it may be hard to work on it for you as there is no documentation how it works internally and many thing have changed.

I will need help with validators and I will start new issue on github for that. I have a problem with choosing the right way of defining validators. I will tell more soon.

talha-asad commented 8 years ago

I like the first approach for behaviors as well, seems it is future-proof as well. Like potentially being able to supply more than options seems easier with that approach.

Whenever you create an issue like that, just mention me and hopefully i'll get along :+1:

talha-asad commented 8 years ago

I just saw this: https://github.com/jagi/meteor-astronomy/wiki/Feature-Requests

Are we going to have ENUM support in Astro 2.0?

Doesn't seem to be in your features list.

lukejagodzinski commented 8 years ago

It's not a top priority, so for now I'm not planning to do it in the first release but if there will be enough time I will implement it.

talha-asad commented 8 years ago

Currently in Astronomy 1.0 we don't get .aggregate method on astronomy classes even after adding meteorhacks:aggregate Is this something you plan on fixing in 2.0?

lukejagodzinski commented 8 years ago

Astronomy class is astronomy class, and Mongo Collection is Mongo Collection. Adding meteorhacks:aggregate does not change here anything. It just adds the aggregate method to all collections. It's also worth noticing that it only work on the server. I'm not going to support aggregate operation until it's officially supported by Meteor.

lukejagodzinski commented 8 years ago

I've published 2.0.0-rc.1 version of Astronomy. It still lacks of some features but I've decided to publish it anyway. There will be a few changes in behaviors and indexes modules that can cause API to be changed. To add Astronomy 2.0.0-rc.1 to your project just type in the console:

meteor add jagi:astronomy@=2.0.0-rc.1

The best way to learn about the API (as there is no documentation yet) is by taking a look at this example.

Behaviors for Astronomy 2.0 are not yet published.

You can also play with a demo of Astronomy 2.0 here astronomy2.meteor.com

talha-asad commented 8 years ago

Congrats on releasing rc1 :+1:

laurentpayot commented 8 years ago

Great, I'm going to test it asap :+1:

talha-asad commented 8 years ago

I see that next in the checklist is

Named behaviors. Add multiple behaviors of the same type to the one class

Are these behaviours also going to work for nested objects?

One a side-note, i think the real spelling of behavior are "behaviour". But maybe i am wrong lol.

lukejagodzinski commented 8 years ago

Yes behaviors should work on nested objects but probable not in all use cases we're discussing here on GitHub. I will tell more when I will be working on it.

behaviors and behaviours are both correct behaviors is American English and behaviours is British English https://www.diki.pl/slownik-angielskiego?q=behavior

sergiotapia commented 8 years ago

:rocket: Awesome stuff, I'm using Astronomy 1 in my project. When do you think 2 is going to land for production use? I'd rather update sooner rather than later. when my models are few.

lukejagodzinski commented 8 years ago

It's hard to tell when it's going to be released. I have a lot of work to do also beside my regular work and Astronomy. On the other hand I have to release it as soon as possible because we will be using Astronomy 2.0 in our products at Useful IO. So the pressure is quite high. It will definitely be Q1.

sergiotapia commented 8 years ago

Cool stuff, we're really excited to try it out here on our large Meteor app. Astronomy feels really natural to use in a Meteor app, should be brought into core.

willrbc commented 8 years ago

Starting to adapt my models using RC1, we will definitely want to use it as soon as possible so keep up the good work :)

One issue I can't seem to solve without documentation is declaring a field as a 'blackbox' Object. Allowing any sort of object to be put into a field.

If I use type: Object I get "Type of the field has to be scalar, class, list of scalars or list of classes"

Is this going to be possible with 2.0?

lukejagodzinski commented 8 years ago

@willrbc I haven't decided yet if it will be possible. For now it's not because I've decided to be compatible with GraphQL in which you have to declare types for each field. Probably it could be possible by creating custom type which is also possible in GraphQL. For now I would like to know as many use cases as possible to get know if such feature is needed at all.

marbemac commented 8 years ago

We absolutely need to be able to define objects without defining their schemas. This useful because:

a) We don't necessarily want to define astronomy objects for every single property all the way into our nested models - it's overkill in some scenarios. b) Mongo is great for storing arbitrary objects, and requiring us to define, upfront, every property in every model means we can't take advantage of this.

For example, we store http request/response data in one of our models, and the response bodies can be json objects of any format.

lukejagodzinski commented 8 years ago

I will probably introduce Object type where it will be any JSON object. However when talking about "any" type or "lack of type" I think it can be avoided.

fields: {
  fieldName: Object
}
marbemac commented 8 years ago

Yup that's perfect.

rdooh commented 8 years ago

It's been a little while since I looked at GraphQL, but my sense is that when creating custom object types, you still wind up explicitly defining things all the way down to scalar leaf nodes... so not sure how easy that will be to allow fuzzy areas in the schema and still keep it GraphQL-friendly.

marbemac commented 8 years ago

Perhaps leave it up to the user to define objects in their totality if they're using astronomy with graphql? You could even log out optional warnings when the user tries to combine generic objects and graphql

rdooh commented 8 years ago

Yeah, maybe you could 'opt out' of a strictly GraphQL-friendly and instrospectable default at the schema level, similar to the case of having 'required' as a default. You could still be partially compliant, but control whether errors are thrown as you suggest.

marbemac commented 8 years ago

That works. In general, graphql is still an area of great thought and change in the Meteor community. I would suggest designing astronomy v2 with an eye towards possible graphql support in the future, but not get too bogged down on it now.

willrbc commented 8 years ago

Thanks, I agree with @marbemac, a JSON object is all we need. That would give us the flexibility we need. At the moment I need to create an Object field called Legacy which I can store an assortment of info from data imported from a legacy SQL database, (such as identity fields of related data etc). Realistically I could define all the fields I need for this but I didn't as it's an edge use case, more needed for testing / development than production.

talha-asad commented 8 years ago

I agree with @marbemac too. Support for arbitrary objects should be allowed. The disadvantage being that class maybe doesn't get GraphQL support.

lukejagodzinski commented 8 years ago

I will allow the Object type and it should work with GraphQL as well. I've consulted it with @ianserlin and GraphQL is only used to fetch data. It does not care about database structure. So it a job for the resolver to provide data in a format that user is requesting. So it won't be a problem to allow any JSON data for a field.

rdooh commented 8 years ago

Ah, so Astro and the GraphQL layers will still be well separated. No problem then :).

lukejagodzinski commented 8 years ago

I've released 2.0.0-rc.2.

Here is a list of new things:

fields: {
  firstName: {
    type: String,
    resolve: function(doc) {
      return doc.lastName; // Don't make sense but it's just to illustrate how it work :)
    }
  }
}

List of changes:

new User({
  'address.city': 'San Francisco'
});

Instead, you should do something like this:

var u = new User();
u.set({
  'address.city': 'San Francisco'
});

In fact using class constructor to initialize document shouldn't be overused. It's mostly used by the find() method.

I haven't done a lot this week. But in the following week I will try to do more.

ianserlin commented 8 years ago

Congratulations!

Can you share a bit more about why you wouldn't want to pass objects to the class constructor, or what we should do instead of using the class constructor altogether?

sergiotapia commented 8 years ago

Congrats @jagi, this is going so well I would not be surprised if it's merged into Meteor core. The missing data layer.

lukejagodzinski commented 8 years ago

@ianserlin ok, so let me give more background on that.

I've done that because of the resolve function which I've introduced. Here is what I do in the constructor:

let fields = Class.getFields();
_.each(fields, function(field) {
    doc[field.name] = field.resolveValue(plainDoc);
});

I'm using the resolveValue method for each registered field in the schema. So constructions like address.city would not work. However, as I'm thinking about it right now I could move this code to the find() method where the resolver should actually be executed. Hmm I will probably change that.

@sergiotapia I don't think that MDG is going to merge any external package into core but from what I've heard they gonna be working on the "data layer" soon so as Sasho Stubailo said they will contact me and Aldeed to talk about data layer.

lukejagodzinski commented 8 years ago

I've published 2.0.0-rc.3 with some minor changes:

and I've done little cleaning of the old code. More new stuff to come.

sergiotapia commented 8 years ago

:fire: Getting closer and closer... :rocket:

ianserlin commented 8 years ago

@jagi thank you!

lukejagodzinski commented 8 years ago

@sergiotapia yep :) @ianserlin no problem :)

lukejagodzinski commented 8 years ago

I've introduced bug in 2.0.0-rc.3 so I had to fix it and add more tests (because of bad tests I haven't noticed it). So I've published 2.0.0-rc.4. Sorry for that. I hope it won't be 2.0.0-rc.99 until 2.0.0 release :D

lukejagodzinski commented 8 years ago

I have a question to all of you about events. It's related with the timestamp behavior. The question:

Now I will give some details, why I need answer to these questions. I think that these events should only be triggered when there is actual save happening (document is modified - there is anything to be saved).

Let's say we have the following code:

var doc = Doc.findOne();
doc.save();

We haven't modified anything but both beforeSave and beforeUpdate events will be triggered. Now in the timestamp behavior I will need to check if there are any modifications and only set the updatedAt field when needed.

Now, let's say that we won't trigger these events until we do something like this:

var doc = Doc.findOne();
doc.name = 'New name';
doc.save();

So, only when we make any change the beforeSave and beforeUpdate events will be called. In such situation in the timestamp behavior, we don't need to check anything.

So what do you think. I think the second approach is correct and it shouldn't cause any problems. Please, try thinking if such change would make any difference to your existing applications. In Astronomy 1.0 the first solution is used. And for now in Astronomy I'm also using it. But I want to switch to the second one, which seems to be for me more appropriate.

What do you think?