-
Notifications
You must be signed in to change notification settings - Fork 84
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
Future add-ons #36
Comments
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. |
Yes, I like this method for creating plugins. However I have a concern about bad quality plugins playing with the main instance of |
yeah, a 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 :) |
I'd love to have the green light to go hack and happy to make it to a 1.0.0 version 👊 it would automatically close at least 5 open issues |
What do you have in mind exactly? Complete rewrite? Breaking changes? Let's discuss the major implications... |
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 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 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 {
} |
Interface-wise I was thinking we should have something like this:
A few notes:
Those are all proposals, let's iterate and reach a sound design. |
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 in ES6, doing a 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 |
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 😛 . 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? |
there's a fundamental flaw in keeping 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
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 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): lodash/lodash#1193 (about wrapppers) An extendable ES6 class MUST be defined as 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 |
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. |
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. |
Sounds great! |
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 |
Looks good so far! Good job! It's nice you took into account most of the open issues. |
👍 |
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? |
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 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 |
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. |
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 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 |
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. |
Yeah, I was just waiting for your feedback on my post above. Should I implement the JSCS to keep format consistent? (including PRs) |
Yes, it's a great idea |
@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 :) |
Hi @pocesar I also want to go ahead with 1.0 😄 , 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? |
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. |
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
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 |
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. 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. |
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 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 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. ES6/ES2015 (see big frameworks like Angular and React) now provides inheritance, decorators and everything to create maintainable code. 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 |
@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 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 |
@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. |
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-deepso 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
The text was updated successfully, but these errors were encountered: