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

Plan for v2.0.0 #106

Closed
19 tasks done
maritz opened this issue Jul 4, 2016 · 24 comments
Closed
19 tasks done

Plan for v2.0.0 #106

maritz opened this issue Jul 4, 2016 · 24 comments
Assignees
Milestone

Comments

@maritz
Copy link
Owner

maritz commented Jul 4, 2016

At long last I am planning to work on a v2.0.0. My current goal is to have this done this fall.

Must haves:

  • "Rewrite" using Typescript and strong coding conventions (tslint)
  • use Promises instead of callbacks (maybe keep callbacks as fallback)
  • Break backwards compatibilty if it brings any advantage 🏃
  • Update README.md and include link to 0.9 Readme until v1 is live
  • Write migration guide - HISTORY.md contains breaking changes and how examples on how to do it now
  • improving logger behaviour (currently console.log) (see point directly below)
  • add documentation for monkey-patching nohm.logError
  • Improve error handling
  • Improve documentation (docco or jsdoc)
  • Unique Validations prevent a Model from being updated. #82
  • work through all "TODO for v1" comments
  • use of the debug module

Should haves:

Research and categorize:

Discussion points:

  • re-evaluate everything that went wrong so far and make sure a v1.0.0 does not have the same pain points
  • Access control

While this is going on I also want to have the admin interface in a state that is at least read-only complete for v2.0.0.

Discussion & feedback & ideas are welcome.

Symbol Meaning
🏃 Work in progress
Canceled
Moved to different release

This was previously labeled as v1.0.0 but has now been changed to v2.0.0 to keep compatibility for dependents.

@sam2x
Copy link
Collaborator

sam2x commented Jul 4, 2016

Good initiative Moritz! I'll help you as much as i can. From my nohm experience, here my feebacks :

  • As i said a lot, i'm using zset for relation, it is very useful only for one-to-many, many-to-many relations (eg: in my friends list feature, i use it to have timestamp of relation), could be cool to have it natively.
  • On my project, I had to build a small layer on top of nohm to handle "safely" nohm*CRUD operations. This layer allow me:
    • To bring Access Control Fields on CRU operation (which imho is missing to nohm)
    • To logs errors (model/entity.not found, validations errors), i had to build an ugly 'hack' to have personal validator error message per model. This need to be reworked imho

Here my recommandations about the evolution of Nohm, some features that may bring this framework to the next level :

  • Better/Easier errors handling (validators, link, and unfound model error.)

  • Access Control List with granularity in the model definition, examples :

    • A model named "User" without role can only read field X, can't create/update/delete;
    • A model named "User" with role ADMIN can CRUD model instance;
    • A model named "User" with role VENDOR can create field X,Y,Z, Read field W,X,Y,Z, Update X,Y,Z, but cant delete the model;
    • A custom rule to be defined in the model definition, and let an argument to be passed to load/factory/save which will be evaluated by this custom rule.
      • Examples 1:

          rules:[
             { desc:'Rule 1 - User admin can read X,Y,Z, write Z, etc. '
               model: 'User',
               roles: 'ADMIN' // that will check that User has a relation named ADMIN with the current instance model)
               link_name: 'role' // optional, default is 'role' 
               permission:'CRUD',
               // or with more granularity, will be used if specified (* => all, '-' except) 
               //permission:{read:['*', '-X'], write:['Z'], etc.}
             }, 
             { desc:'Rule 2 - Custom rules that will allow delegate some task to specific user',
              fn: custom_check,
            }
          ]
          [.. somewhere in the model definition ...]
          /*  instance: instance of the current model (loaded or created)
              context: define the context of the operation; read(model was loaded), create(save was called), update(save was called), a delete(remove was called), maybe useful for complexe check or loggings
              obj : argument passed to load/factory/save depending the action
          */
          custom_check(instance, context, obj, next){
                if (instance.p('delegate_id') == obj.id next({read:['X','Y','Z'], save:['Y','Z'], update:['X'], delete:true}
                else next(false)
          }
        
  • Pub/Sub feature integrated to the model definition (could be scoped to property or model, note: create/delete only scoped to model) :

        {
             properties: {
                 [...]
                 isOnline: {
                       type:'boolean',
                       // parameter notification
                       subscribe:{
                            read:null,
                            update:notifyFriendNetworkStatus,
                       }
                 }
           };
           // whole model notification 
           subscribe:{
               create:[callback1, callback2],
               delete:callback1,
               read:null,
               update:null, 
           }
        }
    
  • Having an Instance of the entity as a parameter to the validators (adding a priority field for the chains validations flow?). Related to Validating properties against other properties #94

        {
             properties: {
                 [...]
                 sub_category: {
                       type:'string',
                       validations:[
                               verifyCategory = function(entry, value, options, next){
                                       console.log('instance being processed:', entry);
                                       next(myconf.list_cat[entry.p('category')].indexOf(value) >= 0)
                               }
                       ], 
                       priority:0,
                 } 
                 category:{
                        type:'string',
                        priority:1,
                 }
        }
    
  • Having Geodata feature in the model definition:
    - a new field type 'geo', which will contains plain geographical coordinate,
    - a new field definition (as like type/validation/defaultValue, etc.) named 'geotable'. Its string value which will be used as redis key to create the geo table (http://redis.io/commands#geo). When this plain hash field value will be created/updated, call geoadd to update the geo table defined.
    - add inherent method georadius/geoPos etc. to the model, eg:

      // will list all model ids within 10km of lat/lng radius
      Model.georadius('field_name', {lat:lat, lng:lng}, '10km') 
      // algo : read geotable value of field_name, call 'georadius' command with args
    
  • Transaction safe ? Does nohm automatic manage dependencies with transactions(redis-multi)? #64

  • Backup rules via nohm and per models ?

  • Having your admin interface being restful compliant ? (have you checked this https://github.com/marmelab/ng-admin ? i use it as client-side interface for my models, it's pretty awesome, and well designed, it uses angular)

  • Documentation : moving to the excellent docco (http://jashkenas.github.io/docco/) for better clarity.

  • Open questions : redis clustering interests via nohm ? interests of lua scripts(Using Lua? #93) ? Benchmark/performance graphs (Benchmark / performance #79) ? TTL feature (time to live (ttl) #97) ?

Hope this remarks are relevants to you.

Regards,

Edit: ps: is this possible to have rights to edit(labels)/close others issues?

@sam2x
Copy link
Collaborator

sam2x commented Jul 4, 2016

I have updated my comment to link issues/questions, correct grammar/typo, and add somes elements.
edit2: added note about doc.

@maritz
Copy link
Owner Author

maritz commented Jul 5, 2016

I agree on Error handling. The current way is not good.

I'm very on the fence about any kind of access control. In my mind this is something better handled by either a seperate module/layer or the application itself.

The pub/sub thing I don't quite understand. What's the legitimate upside of that compared to subscribing to it on the model and just checking if you're interested in the update with 1 if clause? If it's performance, then I think this would be something for a version 1.1 and should be researched/benchmarked as a first step of discussion. Maybe you could create a seperate issue for this?

Instance on validator should already be there in this.
Code for it
Test for it
If it isn't working, that would be a bug as far as I'm concerned. I will admit that it's not documented though.

Geodata to me would be something for a version 1.1 or 1.2. Definitely interested in this though.

Transaction safety: Will at least be explored for a version 1.0. I would take this on as a should, not a must and then a must for a version 1.1.

What do you mean regarding "Backup rules"?

Admin interface frontend is currently written using Marionette and talks to a json API that is REST-ish.

Documentation: Agreed. I personally currently prefer jsdoc with something like docstrap. There already are jsdoc style comments for almost everything, so it would make sense here as well. Current jsdoc output is... not ideal though.

Benchmarks/performance: I still don't really know how to do meaningful ORM benchmarks. But I guess we can try?!

Regarding TTL: My concerns still remain. I'd put this under "to be researched" as well.

@maritz maritz added this to the 1.0 milestone Jul 5, 2016
@maritz
Copy link
Owner Author

maritz commented Jul 5, 2016

I see whee the confusion about this in validators might have come from. Since it's been merged in 0.9.8 I think that part is settled?! 😄

@maritz maritz added the Release label Jul 5, 2016
@sam2x
Copy link
Collaborator

sam2x commented Jul 5, 2016

Awesome.

My bad for #94, havent noticed this update(i will review the code to understand correctly the execution flow).

Also, my backup statement was pointeless & off-topic, this is specified at runtime in the configuration file, not the redis client.

About pub/sub, i added it to my code at init phase, in a function with multiple subscribe to multiple model, and differents handler than check what has changed. I just cant find it readable at all, it's maybe just a personal matter on how i achieved/dispatched this.

Documentation: Great, docco was just the first thing i found on google. I have not really remarks about the choice (as long as it is more readable). Just one point : imho docs should be versionned in this repo.

Benchmarks/performance: Let's just start with something simple. As a draft, let's make a tool that each user can send share the result by specifying their cpu/mem (based on https://www.npmjs.com/package/benchmark ?) :

  • 10k & 1M creation/read/update/delete model with differents fields types
  • 10k & 1M model linking
  • Find/Sort pattern in 10k & 1M keys (non uniq/uniq/multiple/integer/string/ etc.)
  • Etc.

Ps: i'll go off on hollyday ten days this Thurday, i will be able to read message, but wont be able to work during this period.

@katopz
Copy link

katopz commented Feb 13, 2017

  • use Promises instead of callbacks (maybe keep callbacks as fallback)

I think it should be nicer with async/await for this one.

@MegaGM
Copy link

MegaGM commented Feb 13, 2017

@katopz Actually, you're free to use async/await with Promises

@sam2x
Copy link
Collaborator

sam2x commented May 25, 2017

I'm writing a benchmark for CRUD nohm, and I came across a redis client lib benchmark. Actually there is 3 mains nodejs redis-client : node-redis (used by Nohm), io-redis, Thunk (wasnt aware of it)

io-redis and thunk seems more featured than node-redis.Thunk redis, got specifically my attention :

  • Full featured redis commands (cluster, etc.) ;
  • This benchmark test show impressive results between thunk, node-redis and ioredis ;
  • Promise based

Could it be a good idea to move from node-redis (which looks less maintened than the other 2) to thunk ?
If Thunk mimics "API" name of node-redis (or requires small modifications), could be very interesting to test my Nohm benchmark against the two libs.

@sam2x
Copy link
Collaborator

sam2x commented May 27, 2017

Benchmark done for CRUD model & link : #110

@hfwittmann
Copy link

Transactions would be great!

@maritz
Copy link
Owner Author

maritz commented Dec 5, 2017

Typescriptification and Promisification is done.

There are still more things that could be done for the typing. Created #118 for that.

I'll publish this as 1.0.0-alpha.1

If anyone is interested in migrating something to alpha.1 in a test environment, I'd be happy to assist. Currently there is no real guide for changes done in v1, except a small list I tried to keep in v1_changes.md. The best place to start might be looking at the example/rest-server, since that is finally up-to-date again.

@maritz maritz self-assigned this Dec 5, 2017
@maritz
Copy link
Owner Author

maritz commented Dec 5, 2017

TS+Promise version is published as nohm@1.0.0-alpha.1 and nohm@alpha

@maritz
Copy link
Owner Author

maritz commented Jan 1, 2018

To prevent breaking dependent packages/apps, I think this will have to be v2.0.0 and I'll tag the current 0.9.8 as v1.0.0 before releasing v2.0.0. Because v0.x.x is not defined as a stable API according to semver.

nohm should have been at v1.0.0 a long time ago, but I just never did the jump. Since this is only a naming thing, I don't think it'll cause many problems.

@maritz maritz changed the title Plan for v1.0.0 Plan for v2.0.0 Jan 1, 2018
@maritz maritz added the v2.0 label Jan 1, 2018
maritz added a commit that referenced this issue Jan 15, 2018
For the reasoning behind the v1 to v2 change see #106 (comment)
@maritz
Copy link
Owner Author

maritz commented Jan 15, 2018

2.0.0-alpha.6 published as tag alpha

@maritz
Copy link
Owner Author

maritz commented Jan 17, 2018

2.0.0-alpha.7 published as tag alpha

@fatfatson
Copy link

where's the doc for v2.0.0 ....

@maritz
Copy link
Owner Author

maritz commented Feb 10, 2018

Not written yet. But you're welcome to help.

@maritz
Copy link
Owner Author

maritz commented Apr 6, 2018

edit: fixed now, see comment below this one.

@fatfatson https://github.com/maritz/nohm/blob/gh-pages-v2/index.md is the new documentation. As long as v2 is not done it will not be 100% though and the styling on that page is obviously broken and can only be fixed by moving it to the proper gh-pages branch.

There is however a guide on (hopefully) all breaking changes in the HISTORY.md

Any feedback or documentation bugs is very welcome.

@maritz
Copy link
Owner Author

maritz commented Apr 6, 2018

Nevermind, I published it on https://maritz.github.io/nohm/index_v2.html now, so the styling and formatting is fixed.

@maritz
Copy link
Owner Author

maritz commented May 26, 2018

I have decided to leave out custom validation files in v2.0 and instead move the re-introduction of them to either 2.1 or 2.2, not sure yet. (#132)

It's a feature I'm not using myself and that is actually kinda problematic in some ways.

In addition custom logging has been removed from the plan. Instead documentation should just point out that you can monkey-patch nohm.logError with your own function.

With those things in mind and the old v0.9 finally being pushed to v1 I will probably make proper v2.0.0 release in the next few days.

The original plan was (now obviously) too ambitious for v2 and lots of things have been moved or just cancelled. However the improvements in code quality and usability have been very well worth it for me personally.

@maritz
Copy link
Owner Author

maritz commented Jun 1, 2018

Will release v2.0.0-alpha.12 as v2.0.0 and tag it as stable on Sunday if nothing else pops up.

@maritz
Copy link
Owner Author

maritz commented Jun 8, 2018

Some things popped up. Let's try v2.0.0-alpha.13 this weekend.

One thing I only realized today: when doing npm install nohm it has been installing v2 for a while now, because v2 has been tagged as latest because it is the default when publishing.

For some reason I thought npm would install stable by default, not latest. That is definitely not what I intended. Sorry for any inconvenience this may have caused.

But at this point I'm not gonna re-tag v1 to latest, because v2 should be ready for release now.

@maritz
Copy link
Owner Author

maritz commented Jun 10, 2018

Published as v2.0.0 and marked as latest and stable.

API docs have been re-introduced at https://maritz.github.io/nohm/api/index.html

@maritz maritz closed this as completed Jun 10, 2018
@fatfatson
Copy link

congratulations!

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

No branches or pull requests

6 participants