mariocasciaro / object-path

A tiny JavaScript utility to access deep properties using a path (for Node and the Browser)
MIT License
1.06k stars 84 forks source link

Future add-ons #36

Open pocesar opened 9 years ago

pocesar commented 9 years ago

Leaving a clear way to add functionality to object-path without having to hack it (like in cases #30, #22, #21, #18), so it can be extended like lodash does. it has a lot of utility built-in, and you can for example, extend it to use fast.js using internally through _.mixin, like this for example https://github.com/marklagendijk/lodash-deep

var _ = require("lodash");
_.mixin(require("lodash-deep"));

so whenever someone come up with a new functionality, he can just snap it in object-path without hassle. it would expose the internal functions of objectPath to the mixin constructor (like get, del, set, isEmpty, etc)

The point is, the library managed to get 164.000 downloads in a month, which is pretty nice and people are actually using it, so the argument is based on the number of people that could improve it without adding it to core

pocesar commented 9 years ago

I was thinking of something like this. You can extend objectPath and receive an "object environment" when you pass in a decorator function (like angular does):

// promise-object-path.js
module.exports = function(internalStuff) {
    // internalStuff.get / set / del / has / isEmpty / isNumber / etc

    return {
       setPromise: function(obj, path, setValue){
           var self = this;
           if (internalStuff.hasOwnProperty(setValue, 'then')) {
                // assume promise
                return setValue.then(function(value){
                    return self.setPromise(obj, path, value); // keep solving promises
                });
           }
           return Promise.resolve(internalStuff.set(obj, path, setValue));
       },
       get: function(obj, path, defaltValue){ // decorate original objectPath.get 
          defaultValue = defaultValue !== undefined ? defaultValue || Number.NaN;
          return internalStuff.get(obj, path, defaultValue);
       },
       tryGet: function(obj, path) {
           var value = internalStuff.get(obj, path, Number.NaN);
           if (value !== value) {
               throw new Error('Path doesn\'t exist in object');
           }
           return value;
       }
    };
};

// later
objectPath.extend(require('promise-object-path'));

objectPath.setPromise(obj, ['some','path'], Promise.resolve(1)).then(function(oldVal){
});

try {
   objectPath.tryGet(obj, someArrayPath);
} catch (e) {
   // deep path doesn't exist
}

does it make sense @mariocasciaro? (not sure if the examples are functional, created them from the top of my head). the internalStuff is kept intact, all functions attached on objectPath global can be redefined / decorated, as they are just a reflection of internal funcionality.

mariocasciaro commented 9 years ago

Yes, I like this method for creating plugins. However I have a concern about bad quality plugins playing with the main instance of object-path. I know that people like to have everything in one place, but I'm wondering if we should just facilitate the wrapping of different instances of object-path rather than the extension of the main one?

pocesar commented 9 years ago

yeah, a (new objectPath()).extend could work, make it a prototype function instead of a static member of the objectPath global. plus it could have it's own set of options, that would automatically solve my own problem on #27. but people expect globals to have all their functions attached to it (perpetuated glorified globals by jQuery, lodash, underscore, Backbone, ExtJs, etc)

but we IMHO can't stop people from writing up bad plugins (see NPM and Bower, full of them), so that's out of our control, as long the core is solid we are good :)

pocesar commented 9 years ago

I'd love to have the green light to go hack and happy to make it to a 1.0.0 version :facepunch:

it would automatically close at least 5 open issues

mariocasciaro commented 9 years ago

What do you have in mind exactly? Complete rewrite? Breaking changes? Let's discuss the major implications...

pocesar commented 9 years ago

the current global won't change. the objectPath global will be an instanced version of the internal objectPath class with default options. under the hood, the average library consumer won't notice anything different, and it will continue to work as it should in 0.x. so no breaking changes in this aspect. The only breaking change is that the "binding" to object would have to be made to a method, like fromObject.

every current public method (get, del, empty, set, ensureExists, etc) will be just a reflection of internal functionality, that will be kept intact regardless of how much you change those public methods. and to ensure that it's kept intact, the extend call will always receive a deep copy of the 'base' objectPath functions, so you can only append and never remove core functionality.

so, thinking in a pseudo code:

const defaultOptions = {
   someDefaultOption: true
};

function hasOwnProperty(obj, property){
   return Object.hasOwnProperty.call(obj, property);
}

function isEmpty(){
}

function get(){
}

function del(){
}

export class objectPath {
   constructor(options = {}) { 
      this.options = Object.assign({}, defaultOptions, options); // deep copy
   }

   del(){
     return del();
   }

   get(){ 
      return get();
   }

   fromObject(obj) {
      /* what objectPath({}) does today */
   }

   extend(extender) {
     var deepCopy = {
       get,
       set,
       ensureExists,
       coalesce,
       del,
       isEmpty,
       hasOwnProperty,
       options: this.options,
       /*...*/
     };
     Object.assign(this, extender(deepCopy)); // instance assign
  }
}

var instance = new objectPath(defaultOptions);
export default instance;
// end of objectPath module

// another file, let's say index.js
import objectPath from 'object-path.js'; // can be instantiated from scratch

var myObjectPath = new objectPath({myOptions: true});

myObjectPath.extend(function(base){
   return {
       doesntMatter: function(){
          if (base.options.myOptions === true){ 
              // blow up
          }
       },
       get: function(obj, path){ 
          return base.get(obj, path, Number.NaN);
       }
   };
});
var another = new objectPath();
another.doesntMatter(); // doesn't exist, it's an instance method from myObjectPath only, no conflict

we can then split the library in smaller helpers (but keep the less used ones for backward compatibility, like coalesce), without compromising the solid core. things like throwing exceptions on not found paths, dealing with functions, binding stuff to contexts, decorating existing methods, like the decoration of get I included in my example, could be made into plugins.

the nice part of exposing the class from objectPath is that, in an ES6 context, you can directly extend the class using:

import { objectPath } from 'object-path.js';

class OB extends objectPath {
}
mariocasciaro commented 9 years ago

Interface-wise I was thinking we should have something like this:

var op = require('object-path')

//normal usage
op.get({}, ...)

//bind
var myObj = op({})
myObj.get(.....)

//extend the global object, not create a new one
op.extend(
  'plugin1',  // the common.js module name of the plugin
  plugin2,   // a function reference (e.g. from a plugin)
  function extender(base) {  //a function defined in place
})

A few notes:

Those are all proposals, let's iterate and reach a sound design.

pocesar commented 9 years ago

Well, I'm thinking ahead, when ES6 is the norm. The way I proposed, you can still use extend once and it will be global (since you are modifying an instance, that is a global, in this sense, it's not creating a new object, it's always using the same reference). and since we are using semver, it should be fine to push it to a major version which means a breaking change of some sort, the Javascript ecosystem is used to that, mainly because of bower, component and npm. Refactoring from objectPath(obj) to objectPath.bound(obj) should be a no-brainer

in ES6, doing a class myClass extend objectPath is a good example of good design that encourages composition instead of singletons, which objectPath is at the moment.

I liked the automatic you presented in extend, passing strings and automatically require the needed modules, the problem is that, in the browser and in AMD they might not behave the same they do in a node.js environment with common.js. It's a shortcut for extend(require('some'),require('other')), but saving a few require typings is really worth it?

mariocasciaro commented 9 years ago

Your reasoning makes perfect sense, you are free to create a 1.0 branch and start working on the new implementation you proposed but at one condition :stuck_out_tongue: . I like the current interface where the default exported function binds the library to an object, it makes perfect sense and it's a pattern used by other libraries such as lodash and underscore. Changing this particular interface is like going in the wrong direction, besides introducing a breaking change. It's simply not worth changing it IMO. What do you think?

pocesar commented 9 years ago

there's a fundamental flaw in keeping objectPath({}), that is the anti-pattern of returning from inside a constructor. the "wrapping" (if we may call it that) is (ab)using the fact that functions are instantiable with the new operator, and can act as objects (have properties attached to it other than the prototype).

lodash / underscore, backbone, jquery, aren't actually good examples of good patterns nowadays, they are "too big to fail", that means they are used by so many people that removing something is outrageous (check the "hack" in https://github.com/lodash/lodash/blob/es/chain/lodash.js#L107-L117 having to manually resort to adding "privates" using __chain__ and __wrapped__).

objectPath({}) was added in January 29 on version 0.9.0 which shouldn't be a widespread panic lol. if people know semver, they will understand that nothing will change if they want to stick with 0.9.x and there will be no drawbacks / refactoring / recoding at all, and still have their projects working fine.

for example, the change from #27 really screwed me, and I'm stuck with 0.7.x in my project, because there's no easy refactoring from objectPath.get(object, ['type', where, 'something.dotted'].join('.')), if there's no options coming for 1.x, I'm stuck with 0.7.x forever. that can't be compared from using var h = objectPath(obj); to var h = objectPath.bind(obj);, since the usage of the generated instance will be the same h.get('some.path'), h.del('ok.ok'), h.ensureExists(['nothing','changed']). In my case I cited in #27 I would have to find all objectPath calls and analyze them thoroughly to see if I'm calling it using anything with a number in it.

adding options to objectPath can make callbacks, event emitters and such much easier. for example, you want to "listen" for undefineds:

objectPath.extend(function(base) {
   return {
      getCb: function(obj, path, def){
          var ret = base.get(obj, path, def);
          if (this.options.callback) {
             this.options.callback(obj, path);
          }
          return ret;
      }
  }
});
// without options, we would have to provide a callback to each objectPath.getCb();
objectPath.options.callback = function(obj, path){
    console.log('Undefined', obj, path);
};

// maybe enable a trace for wrong paths
objectPath.extend(function(base) {
   return {
      getTrace: function(obj, path, def){
          var ret;
          try {
             ret = base.get(obj, path, def);
          } catch (e) {
             if (this.options.debug) {
                this.options.debug(e);
             }
             return def;
          }
          return ret;
      }
  }
});

objectPath.options.debug = function(e){
    console.log(e.stack);
};
objectPath.getTrace(undefined);

some posts to shine some light on the discussion about globals, unextentable 'classes' and the (near) future that's upon us (ES6):

https://github.com/lodash/lodash/issues/1193 (about wrapppers) http://discuss.emberjs.com/t/best-practices-shimming-libraries-which-use-global-variables/7922 http://benmccormick.org/2015/04/07/es6-classes-and-backbone-js/ http://ludovf.net/reactbook/blog/reactjs-es6-classes.html

An extendable ES6 class MUST be defined as class ObjectPath { } and have a constructor and a prototype. If we want to be able to do class MyObjectPath extend ObjectPath { }, it must be instanceable. That's the main reasoning why I'm favoring the rewrite towards a base class and a global instance of this base class. The goal is to be able to change and embrace the future while we can. The number of monthly downloads are getting higher.

we could even add the plugins to the wiki, and add them as people make them, creating a mini-repo of plugins here, and picking a standard tag for npm and bower, like gruntplugin2

mariocasciaro commented 9 years ago

I guess 300 lines of code are not worth all this back and forth of opinions. In short, let's go ahead with what you have in mind :), I trust your judgement otherwise I would not have given you write access to the repo :). Let's go ahead with the 1.0.0 branch.

pocesar commented 9 years ago

alright, I'll be using the 1.0.0 branch I created. I'll try to do the benchmarks and the 1.0.0 this weekend. I can also document the code using jsdoc if you are fine with it (IDE users would be grateful)

mariocasciaro commented 9 years ago

Sounds great!

pocesar commented 9 years ago

Preliminary rewrite done (the basic funcionality). Extend not done yet

https://travis-ci.org/mariocasciaro/object-path/builds/62853434

Tests weren't changed, only for objectPath({}) -> objectPath.bind({}). I'll then comment the code, add the rest of the tests and get 100% coverage, then you may review it @mariocasciaro

mariocasciaro commented 9 years ago

Looks good so far! Good job! It's nice you took into account most of the open issues.

pocesar commented 9 years ago

:+1: pushed the "release candidate" to the 1.0.0 branch. all tests passing include more use cases from the tickets, but I'm missing the prototype inheritance test using the objectPath.Class.call(this, options) when constructing your own class derived from it

mariocasciaro commented 9 years ago

Could you give me a quick overview of how the typescript file relates to the .js one? Are they 2 totally different sources (2 different sources to maintain manually and keep in sync) or the .js is the result of a compilation step?

pocesar commented 9 years ago

hey @mariocasciaro. Typescript is a superset of Javascript (Typescript is valid Javascript when it's not annotated (using the variable types), and Javascript is 100% valid Typescript, unlike other transpilers like Coffeescript) that allow strict variable types to catch program errors before it's transpiled to plain javascript (from .ts to .js). By defining a function with fn(variable: string), you'll never, at compile time, be able to call it as fn(10). Typescript also "forces" you to plan all the library behavior upfront, so you won't be caught in weird use cases.

The only thing that need to be kept is the .js. The .ts serve as the base that will always be kept in sync with the JS when it receive patches (by me of course), so the typings (the .d.ts file) is kept up-to-date. To understand why the typings is such a big deal, check this short post: http://www.johnpapa.net/intellisense-witha-visual-studio-code/

mariocasciaro commented 9 years ago

I don't have anything against Typescript itself, but the fact that we have to maintain 2 sources in sync is not optimal. That's why I was asking if the two files were linked in some way (e.g. compilation step). What are the practical downsides of using the js version of object-path from a Typescript file? By 'practical' I mean things that gets broken, lack of features or non negligible loss of productivity. What I'm really asking is: is it really worth the effort? I prefer a library super easy to read and maintain than something that we will hate to even look at in 2 years.

pocesar commented 9 years ago

Well, I've been using typescript for almost 2 years now, and it helped me imensely to write quality code. But don't worry, I can keep the Typescript on my repo only, and never push it to this repo. I'll need to maintain the typings up-to-date anyway, regardless if this repo keeps the .ts version or not (all typings belong to one repo, that is DefinitelyTyped).

To compile the typescript, use npm install typescript -g then tsc inside the cloned folder, no other step is needed, if you are curious how big would be the impact of compiling.

Typescript will always be 1 step ahead the current Javascript version, so it won't be "hateful" to look at it in the future, it's valid Javascript (ES6 > ES7) if no annotating is used. Angular 2.0, the biggest upcoming front end framework is done with Typescript, so you can measure how big it will become, and the direction having two of the biggest internet related companies (Google and Microsoft) will lead in the community.

One thing that should be setup though is JSCS, it applies code styles and would require an additional 'build' step, to make the code as readable as possible (either using grunt or gulp) regardless of the input (https://medium.com/dev-channel/auto-formatting-javascript-code-style-fe0f98a923b8)

Forgot to mention that out of the box, Typescript can compile code down to ES3, ES5 and ES6 compatible code just be changing the --target command line, so it's future proof for and by itself

mariocasciaro commented 9 years ago

As I said, I don't have anything about Typescript, I just don't want to maintain 2 files in sync, it's just unpleasant, error prone and discourages contributions from the community. I vote for keeping the .ts file out of this repo for now, let's focus on a pure JavaScript implementation here, let's keep it simple. Besides this, is the 1.0 ready to go for you? I checked the source and it looks good to me. After this, I think we are good to update the docs and release.

pocesar commented 9 years ago

Yeah, I was just waiting for your feedback on my post above. Should I implement the JSCS to keep format consistent? (including PRs)

mariocasciaro commented 9 years ago

Yes, it's a great idea

pocesar commented 8 years ago

@mariocasciaro Revisiting this issue, the new Typescript 1.6 version can create typings that doesn't need to include the original typescript file. Also, it favors typings encapsulation instead of declaring global modules (like Typescript =< 1.5 did), so I guess it won't need to rely on the code generated by it, unless we go full ES6 (or SystemJS hybrid for example). Still want to release 1.x :)

mariocasciaro commented 8 years ago

Hi @pocesar I also want to go ahead with 1.0 :smile: , my only prerequisite is that we keep this library as simple as possible. For the size of this lib I don't see any advantage in using Typescript or ES6 + a compilation step. I don't have anything against TypeScript or ES6, I just feel that a small library as this should stay small and simple. Adding more features, streamline the internals, improving performances, splitting the library in many submodules and things like that are far more important in my opinion, than debating if we should use Typescript or ES2015 or plain ES5. We agree on this?

pocesar commented 8 years ago

Yup, totally agree. Standing in the shoulder of giants (like lodash / underscore and now their merge) the focus is on splitting the internals into separate exports, so people using ES6 style imports can use only some parts directly like

import {del} from 'object-path';

del(obj, 'path');

I'll refactor it, i'll keep the backport to Typescript and do the typings on my fork and keep the upstream updated, while keeping this repo JS-only.

pocesar commented 8 years ago

hey @mariocasciaro here's the upcoming version. https://github.com/pocesar/object-path/tree/1.0.0

disregard the typescript files for now, and it's missing the global objectPath inside the UMD. the tests are passing, support for Symbol() is nearly done, and I've updated the README Also, I'm proposing the following plugins that might or not ship as soon 1.0 is out:

object-path-as-promised
Adds `objectPath.then`, a getter that returns a promise, and can deal with plain values and promises (thenables)

object-path-concat
Adds `objectPath.concat`, concat if the path is an array

object-path-tryget
Adds `objectPath.tryget`, throws if the path doesn't exist

object-path-wildcard-set
Adds `objectPath.wildcardSet`, can set paths like `some.*.path`

object-path-immutable
Overrides all `objectPath` functions, make it so object-path don't modify the passed objects, but always create a new object or array

I saw that you created a separated version of object-path-immutable to have a clone function, it can now be a plugin and extended into object-path, what do you think? ;)

EDIT: preliminary tests are here https://travis-ci.org/pocesar/object-path

mariocasciaro commented 8 years ago

Hey Paulo, that's an amazing job, I think we are on the right path, however there are a couple of things I wanted to discuss first. Object-path functions should ALWAYS be consistent in behaviour across and within projects. This is not always true if we allow the user to create a new 'customized' instance with different options. Imagine having a project that uses object-path extensively, if every member of a team starts creating their own instance which can behave differently from other instances in the same project it would be a complete mess. It's even hard to document it, imagine explaining the set() method: if you specified option x then you will have this, if option y and z then that. Plugin creators might easily abuse of this. I think this is the wrong way to do it. I thought about this a lot, and I think the right approach would be to create a completely different function for each variation in behaviour, each function should do only one thing and well and state clearly and without ambiguity what is its behaviour, for example we would have set() to treat numbers as array and setObject to always treat the path components as object properties. This philosophy is in line, for example, with what gulp considers a good plugin (a gulp plugin that tries to do many things and accepts many options is considered a bad plugin) Each object-path function would be distributed in its own package (as lodash does) and we will then provide an aggregated library for convenience (object-path which will import all basic functionality). Each individual function will be distributed only as a commonJS module for simplicity, while the main library will be also built with browserify to support those stuck in the 20th century (AMD and browser globals).

Let's start by:

Object-path-immutable fits perfectly with this approach as well as the many plugins you suggested. If the users wants a 'customized' version of object-path they can always do:

var myObjectPath = {
  set: require('object-path-set')
  get: require('object-path-get'),
  getPromise: require('object-path-get-as-promised')
}

This is much simpler and clearer for the user than passing an options object to a constructor and it also gives more flexibility. Let's create a unix-style ecosystem of utilities around object-path, small, simple and powerful at the same time, people will love it.

pocesar commented 8 years ago

hey @mariocasciaro, while I understand your point of view, I disagree with it.

Lodash functions itself have many different outputs for the same function (see reduce, debounce, throttle or clone for example). Even the _() is a factory for an internal class, while it might not appear to be one class for the simplicity of the interface, it is, while all the bells-and-wristles of a prototyped class (and a really useful one, I use the chained class all the time) or through _.runInContext, which I also use in Node extensively. Also, besides the split (and not very popular versions on NPM) for lodash-* functions, the main lodash file encapsulates everything most programmers use and love.

Gulp is a 'one time' build step, and it's expected to be executed once, and do something well, aka process files and output useful modifications. That's not the case with object-path. It's a server and client side runtime utility, that's used extensively within code, over and over. All functions in objectpath do what they are named after, they either get, set, delete, push or empty objects. Using configuration flags won't change drastically the way they work, it will affect non-existing paths mostly, pre-defined objects will behave as expected regardless of flag. Having setObject, setArray creates unnecessary complexity. That's the reason I wanted to use Typescript, the code is really well documented, the parameters are safe guarded, and won't let you commit simple programming mistakes, if you see the base interface for object-path, it's straight forward, and IDE support is superb (even without JSDoc, which I plan to do before releasing 1.0).

vscode1 vscode2 vscode3 vscode4

Also my proposal tries to abide within the S.O.L.I.D. principles, while maintaining some backward compatibility with object-path through exporting each function like it was doing in version 0.x, allows transparent extension for instances, while keep the modifications at bay. That's why besides objectPath.get, which behaves exactly the same every time, there's objectPath.instance.get, that will behave exactly per configuration, the same every time as well. That's what I mean for separate exports. They belong as the core of object-path, but they can be used individually (not separate modules)

ES6/ES2015 (see big frameworks like Angular and React) now provides inheritance, decorators and everything to create maintainable code. class MyComponent extends React.Component { } is much easy and maintainable than defining a "singleton" object that will have to be instantiated later somewhere (which objectPath.instance is, a global instance of objectPath that behaves like the original one). For your "roll your own", you can simply use class ObjPath extends objectPath.Class { } and hack away. Providing ways for programmers to do this is the best.

Otherwise object path will never take advantage of progress that were made in newer versions of Javascript.

What I said was that extra behaviors should become plugins, it's because we provide plugin authors with a straight forward way to create code without even having to require object-path inside their plugins the first place:

// no need require('object-path') here
module.exports = function tryGet(base, options) {
   return {
      tryGet: function(){  
         base.get() // don't need to worry, because get is ALWAYS objectPath.get, no matter what modification was done to either objectPath.get or ObjectPath.instance.get
        // closed for modification, open for extension
      }
   }
}

check the readme for examples

mariocasciaro commented 8 years ago

@pocesar unfortunately we clearly have different opinions on the matter and it seems difficult that we could ever find a compromise. Your arguments make completely sense, but they don't apply to a simple library like object-path. Using a class and keeping a state (the options) is completely overkill for a library that is meant to be intrinsically stateless, simple and made of simple functions.

There are a lot of people who argue that classes and in particular inheritance are harmful and should be avoided. Personally, I'm not completely against the use of classes and inheritance, but if there is a simple way to avoid them then I will.

I won't stop you to continue to work on your own version of object-path, you can always fork it permanently, change name, do whatever you want, however I would hate to see you go, I would prefer to join forces towards a common goal instead of splitting forces to reach the same goal but through two different paths. Remember that this is just a small library, let's not overthink it, ultimately we should focus our time on far bigger and more important things.

pocesar commented 8 years ago

@mariocasciaro I don't plan to fork object-path, it would do nothing good doing so. I'll do as you say, I'll remove anything that is related to classes and state, but I need to ask about the extra parameters added to the functions. numberAsArray isn't as useful as the ownPropertiesOnly, so I can remove numbersAsArray. Like set has doNotReplace, ownPropertiesOnly as a parameter is a powerful one thinking on plugins implementation. It's not stateful, it just indicates that you want to go BEYOND own properties (including symbols and prototype non-enumerables) and you know you can break things, but some plugin authors might find this useful. If the parameter option isn't implemented, then a person that wants a new 'isEmpty' for non-enumerables will have to write his own code. (definition in https://github.com/pocesar/object-path/blob/1.0.0/object-path.d.ts#L29 and subsequent set, ensureExists, del, etc). all ownPropertiesOnly parameters defaults to true and it is the current behavior for object-path