-
Notifications
You must be signed in to change notification settings - Fork 16
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
Remove identical intializers #70
Comments
This seems reasonable. I personally cannot think of any case where you would want the same initializer repeated. If we can think of ANY case where you would want to repeat the same initializer function multiple times, this should not be added as there will be no way to undo the filtering. Can anyone think of such a case? If there is a use case for repeating initializers I think the unique filter should be an option to the user not an assumed need. Thinking out loudIs there a case where you would want to consider 2 initializer functions equivalent in a way other than Implementation ideasImplementing this behavior your self is extremely low effort. let stamp = compose(A, B, C, D);
stamp.initializers = _.uniq(stamp.initializers); Maybe change nothing and add documentation? You could have it both ways: Specify a way to identify which functions should be unique filtered. Such as considering an initializer unique if it is a named function or has a This is similar to the problem I ran into here with naming the composer functions #63 (comment) |
let stamp = compose(A, B, C, D);
stamp.initializers = _.uniq(stamp.initializers);
This gets my vote. In fact, it's a good general rule of thumb. If something can be easily done in userland, it should not be added to the spec. Consider a utility stamp, instead. |
👍 |
Hi @unstoppablecarl and @ericelliott In this issue submitted by @FredyC (Daniel K.) it is described the same problem as I had. Please, take a thoughtful read of the issue. I also wonder why only me and Daniel met that problem... |
Hey, I am just starting to learn about stamps, but I cannot think of any use case where you might want to have same initializer called more than once. Among my first thoughts when I started using stamps was the actual question "what will happen if I add some stamp more than once in there?". I could not find satisfying answer anywhere so I just went with it as it seemed only logical that it should work that way. Since it works with everything else in the package, why initializers should be different?
Frankly this seems very hacky to me. What about doing that in opposite way? By default duplicate initializers would be filtered with a following function. Anybody can override that and change behavior if necessary.
Considering that there will be probably very low number of cases where you want to run initializer multiple times, a cost of increased API surface seems very low. |
I'm all for a |
@ericelliott Can you talk about some of them? As I said, it doesn't really feel consistent this way. Other parts of stamps like methods, refs, etc... can be composed multiple times and nothing wrong happens. Then suddenly there are initializers which can break stuff with multiple call.
Perhaps trivial, but very repetitive. Imagine you would do that for tens of stamps in your codebase. It can get rather tricky and error prone. |
@FredyC In the future releases of the stampit module the hacky way you have mentioned is not only allowed, but even encouraged. Stampit v3 will be implemented (actually it is already implemented, but not yet released) using the specifications from this repository. So that you can manipulate those initializers the way you want. One of the ways to implement unique initializers is to override the ".compose()" static method. Here is an example. const UniqueInitializersComposeOverride = compose({statics: {
compose: function(...args) { // overriding the ".compose" function of any future stamp
const stamp = this.compose(...args);
stamp.compose.initializers = _.uniq(stamp.compose.initializers);
return stamp;
}
}});
const WhateverUniqueInit =
UniqueInitializersComposeOverride.compose(Whatever); // overriden compose()
const MyStamp =
compose(WhateverUniqInit, Foo) // the regular (not overriden) compose()
.compose(Bar, Baz); // overriden compose()
const objInstance = MyStamp(); Please note, that this is not available in stampit v2. Is the example understandable? (as it might be not, because I'm a lousy teacher) |
@ericelliott That initializer will be invoked while stampit is iterating over the same array. So, you probably meant to mutate the const UniqueInitializersInitMutate = compose({initializers: [function(options, {stamp}) {
const inits = stamp.compose.initializers;
const length = inits && inits.length;
for (let i = 0; i < length; i++) {
const sameIndex = inits.indexOf(init[i], i + 1);
if (sameIndex >= 0) {
inits[sameIndex] = undefined; // mutating the array
}
}
}]});
const WhateverUniqueInit =
UniqueInitializersInitMutate.compose(Whatever); // must be first
const MyStamp =
compose(WhateverUniqInit, Foo) // must be first, and it is first
.compose(Bar, Baz); // must be first, and it is first
const objInstance = MyStamp(); Correct? This approach might can have side effects. Although, I am yet to think of one. |
I don't know why all that hassle really. I still haven't seen any use case of why it might be useful to run initializer multiple times. Shouldn't default behavior be like something that is used in 95% of cases while 5% (need multiple invocation) can use hack? Sorry, but however supported it will be in v3, it still feel hacky. You need to search documentation and then figure out how to fit that in your code base. |
Stamps is a new paradigm which is hard to switch to, I know. I felt the same when stopped doing classes and started doing stamps. The things you're calling "hacky" are quite normal and encouraged in stamps.
That's my bad. The
Me too @FredyC! Several times I needed those initializers to be unique. And 0 times I needed them to run multiple times. Just hear the words - multiple initialization of the same thing. WAT? @ericelliott could you please provide few examples of:
|
Reopening as many people request that feature. |
This issue need more opinions. I invite @JosephClay @sethlivingston @troutowicz @yasinuslu @zebulonj to express an opinion if you have one. Thanks! |
I don't have enough experience using Stamps on production scale projects to offer an opinion. Would be interested (pure curiousity) in an example use-case where you'd want to run the duplicate initializer more than once (as @ericelliot mentioned). I can't think of any off the top of my head, but—like I said—don't have much experience with Stamps in production-scale projects. |
I'm OK with dedupe by default. Seems to be the consensus. |
If someone what to submit a PR - your very welcome. |
It makes sense to dedupe if duplicates existing causes trouble more times than not. |
As I said before when we discussed this: I think there should be a way to mark which initializers should be deduped and not. On the other hand, no one has produced an example where you would need to run the same initializer multiple times so maybe it is not an issue. |
I don't know, sounds like trying to prepare for something that might never happen. We are six people here with various level expertise. If none of us can come up with reason why it should be allowed, why not just do it easy way? Only if someone comes with real use case, expand it then instead of thinking way too ahead and creating suboptimal part of code because of that. |
Just because I don't have time to share examples doesn't mean I can't come up with them. =) |
Btw, if one would need to avoid deduping then there are multiple ways to implement it yourself: const initializer = function(...){...};
// 1
const MyStamp = compose({initializers: [
function (...args) { return initializer.apply(this, ...args); } // make a wrapper of the function
]});
// 2
const MyStamp = compose({initializers: [
initializer.bind() // make a copy of the function
]}); |
Please, merge: #89 |
Resolved in #89 |
@koresar I believe that @boneskull has brough up other questions, deduplicating initializers is just current solution, but is it correct one? |
Could be not. I'm ready for other solutions. Please, propose! |
Well I did...although it was crazy. |
So, whether this is right or wrong, in ES6 classes, you must explictly call your superclass's constructor. What if, when composing Stamps, initializer execution had to be explicit? Just tossing out ideas. |
@boneskull What do you mean by explicit execution of initializer? Can you show some example? Since there is no superclass here, you are ensured that initializers of all stamps will always called at least once. If by explicit execution you mean to call some logic of other stamp, then you are better with method I guess. |
Yes, basically that upon composition, merging of init functions would be skipped. Then a stamp would only have one (1) initializer. given you need a stamp to use it in composition, accessing the initializer is trivial: const A = stampit().init(myFunc);
const B = stampit().compose(A).init(function() {
A.fixed.init.apply(this, arguments);
}); Hope that looks right. Tough to code on a phone |
I'm not suggesting this as the syntax but would expect a more covenient api |
And yes I realize this clobbers the API but wonder if it doesn't address a design flaw. |
To achieve that kind of behaviour I'd recommend Infected Compose. import compose from 'somehwere';
function shadowImplicitInitializer(opts, specialOpts) {
// Add one more special options
specialOpts.inits = stamp.compose.configuration.explicitInit.inits;
// Call the only initializer provided by the user
return stamp.compose.configuration.explicitInit.theOnlyInitializer(opts, specialOpts);
}
function explicitInitCompose(...args) {
args.push({ staticProperties: {
compose: explicitInitCompose, // infecting stamps
explicitInit: function (theOnlyInitializer) { // new API function
return compose.apply(this, { // return a new stamp with an additional conf
deepConfiguration: {
explicitInit: {
theOnlyInitializer, // overwritten each time the explicitInit() called
inits: [] // concatenated each time the explicitInit() called
}
}
}
}
}});
const Stamp = compose.apply(this, args); // creating an infected stamp
// Check if we need to wrap the initializers
if (!Stamp.compose.configuration.explicitInit.theOnlyInitializer) return Stamp;
// moving original initializers functions to configuration
let inits = Stamp.compose.configuration.explicitInit.inits;
// remove the initializer wrapper if present
inits = inits.filter(init => init !== shadowImplicitInitializer);
// Take the actual initializers and move them to the configuration
Stamp.compose.configuration.explicitInit.inits = inits.concat(Stamp.compose.initializers);
// Removing actual initializers, place there our only initializer
Stamp.compose.initializers = [shadowImplicitInitializer];
return Stamp;
}
// Usage:
const RegularStamp = explicitInitCompose.init(fn1).init(fn2); // regular stamp
const SingleInitStamp = explicitInitCompose.explicitInit(
function (opts, {inits}) { // accessing the initializers
init[0].apply(this, arguments); // if needed
}
);
// All sort of composition is possible
const MishMash = SingleInitStamp.compose(RegularStamp).compose(
SingleInitStamp.explicitInit(function (opts, {inits}) { console.log(inits); })
);
compose(explicitInitCompose(), MishMash);
// etc ^ totally valid code. Not tested. Could be simplified (shortened) because I'm in a rush. |
Another way could be editing the initializers array while first initializer is running: const ExplicitInit = compose({
staticProperties: {
explicitInit: function(theOnlyInitializer) {
// Adding the new API
return compose.apply(this, { configuration: {theOnlyInitializer} });
}
},
initializers: [function shadowImplicitInitializer(opts, specialOpts) {
const {stamp} = specialOpts;
// One more argument to the special options
specialOpts.inits = stamp.compose.initializers.slice(1);
// Remove all the initializers from the array object
while(stamp.compose.initializers.length) stamp.compose.initializers.pop();
// Call the initializer supplied by the user (TODO: check if it was provided)
return stamp.configuration.theOnlyInitializer.apply(this, opts, specialOpts);
}]
});
// Usage:
const SomeRandomStamp = compose(/* whatever */);
const SingleInitStamp = ExplicitInit.compose(SomeRandomStamp).explicitInit(
function (opts, {inits}) {
init[0].apply(this, arguments); // if needed
}
); This is a simpler implementation. The only caveat with this approach is that the |
Thanks @koresar. Correct me if I'm wrong, but if you were to use "explicit" inits, you wouldn't run into the original problem, right? Worth metioning is that If we were to somehow allow the user to give each init function a name, then the data structure will quickly get ugly. We'd need to consider what if the user didn't supply a name, duplicate names, cost of searching, etc., etc. Ultimately, I'm not really sure that we need to do any of that, unless I'm missing something. What I'm saying is:
Given that when you're composing import {Stamp1, Stamp2, Stamp3, Stamp4} from './stamps';
import {compose} from 'stampit';
const Stamp5 = compose(Stamp1, Stamp2, Stamp3, Stamp4)
// might want to use initOnce() just in case, but let's throw caution to the wind
.init(function () {
Stamp1.initOnceFunc.apply(this, arguments);
// maybe Stamp3 had one we wanted too?
Stamp3.initOnceFunc.apply(this, arguments);
});
// also, all functions registered via init() in Stamp1, Stamp2, Stamp3 and Stamp4
// are called as usual. So, I don't understand why we'd need access to the Again, a very rough API, but I wanted to be clear about what I meant--and this is just a wild idea, unless you think it's a good one, then I'm totally serious about it. 😄 |
Not right. The "compose" function would still concat initalizers without deduping.
Stamps are great because YOU CAN EXTEND THEM WITH ANY API YOU WANT.
In your code example it is a regular function composition. I'd recommend to use functions for that instead of stamps. Otherwise, it's a heavy overcomplication.
Thanks for sharing with us! I'd resume: stamp specification does not need to be extended in order to implement your wild idea. The feature can be implement as a composable behaviour. |
I think I got sidetracked a bit from my original point by talking about an API when we're supposed to be talking about a specification. I was kind of unsure why you wrote all that code... Anyway, I believe what you're saying way up above is that we want to dedupe inits because:
That's fine, but I'm trying to get to why we have this problem in the first place. An attempt to solve it was "let's stop introducing dupe inits", so deduping is no longer necessary. I offered a way to do this without breaking how |
Yeah. You're making total sense. 👍 Have I listed occasions when I actually needed deduped initializers? I think I did so somewhere above. Another reason to dedupe could be performance concerns. But that's secondary. |
Well I must I got bit lost in here what we are actually trying to solve by all this. I mean trying took for different approach instead of deduplicating initiatilizers out of the box, that's obvious. However things like
Sorry if I am wrong, but have you written some complex stamps on your own? As you have stated here, you are deliberately avoiding to compose stamp multiple times over. Perhaps if you start avoiding it, you will discover the need for deduping. I think it's nice feature of stamps, for OOP newcomer it looks like long wanted multiple inheritance. |
I still think if you need to dedupe, that's a code smell. Could you be using shared modules or dependency injection instead of trying to turn everything into a stamp? Stamps aren't magical hammers that turn everything into a nail. |
@ericelliott I still see it as a main feature of stamps that you don't need be afraid to do that. I certainly don't want to assume that some high order stamp is composing what I need in some lower levels. I would rather explicitly compose it to be certain it's there. On the other hand, I do not consider myself such a beginner with stamps now and to be honest I don't really have any example where running initializer multiple times could be harmful except performance reasons. Btw, just out of curiousty, it's already almost 3 months when you've said:
Still have no time to get at least one example together? ;) |
I think you meant to say "if you stop avoiding it". Of course, if I didn't avoid it, given the way Stamps currently work, I'd need to dedupe. But given that I'd need to dedupe, I'm not sure why I'd do it, except...
So, this would be the "why"--but I'd consider this practice to be overly defensive (opinion).
I'm in agreement with the above, regardless of whether there's a use case for duplicate initializers. |
This isn't how composition is supposed to work. There should not be implicit dependencies between stamps. Ideally, stamps know absolutely nothing about each other, and when they do need to know about features of other stamps, those features are shared by common modules or dependency injected so that all your stamp dependencies are explicit. Dependencies should always be explicit rather than implicit. If you have implicit dependencies and/or duplicated init functions, I still argue that's a code smell. Fix the design, not the library. I'm OK with deduping init by default, but I've never personally had this problem, and I've been using stamps longer than any of you. =) |
@ericelliott could you please carefully read this and tell me what you think of the UPD: I'll same you some time. Without deduping this would create 3 identical initializers: const SlackApi = compose(ArgumentChecker)
.checkArguments({team: 'string'}) // 1
.checkArguments({accessToken: 'string'}) // 2
.checkArguments({apiVersion: 'number'}) //3
.compose({initializers: [
function ({team, accessToken, apiVersion}) {
console.log(`${team}, ${accessToken}, v${apiVersion}`);
}
]}); With deduping the above code adds only one argument checking initializer. |
I am not sure what exactly you call dependencies. For example the stamp that wraps EventEmitter. export default const EventEmittable = stampit({
initializers: function initEventEmitter() {
Reflect.apply(EventEmitter, this, []);
},
methods: ['emit', 'on', 'once', 'removeListener', 'removeAllListeners'].reduce((methods, methodName) => {
methods[methodName] = EventEmitter.prototype[methodName];
return methods;
}, {}),
}); This one is than composed explicitly in various other stamps to get those methods on it. Do you call that a code smell? Obviously it's not a nice to have a initializer of that called multiple times. Are you suggesting to create instance of that EventEmittable and pass it in options instead? That would mean you would have to manually expose those methods everywhere. It's probably possible that way, but you know, DRY isn't that stupid idea. |
@koresar Bonus: You'll colocate the required arguments/validation info with the stamp that actually uses them. EventEmitter is also a great example of something that other stamps should not be aware of. If other stamps need to use the emitter API, you can explicitly pass it into the stamp using dependency injection.
Yes. If it is problematic to do so, perhaps we could come up with a simplified API for making explicit dependency injection easier. |
Everybody please read "Mixins Considered Harmful" -- Dan Abramov's warnings about mixins apply to stamps. Stamps are not a panacea. You should not try to use them for every little thing. |
Note that I have no problem with doing stuff like this: import EventEmitter from 'stamp-event-emitter';
import { compose, init } from 'stamp-utils';
const myStamp = compose(EventEmitter, init(options => {
// use EventEmitter here
})); The distinction here is that in this file, the dependency on So... If you then compose this OK use case with another stamp that also uses What you need to watch out for is implicit dependency between modules. Your module should have an Make sense? |
Totally makes sense and that's the way how I am doing it. Whenever I am using some api of another stamp, I am explicity importing it and composing there. That's why I was bit confused by this comment of yours...
So with explicit dependencies the dedupe is no longer code smell I assume? Anyway regarding mixins ... it's bit unclear where mixins end and "right way" begins. Even if I explicitly state that this stamp composes EventEmitter and then I use |
As I mentioned before, I've been using stamps for years -- since before my book was published, and I have never needed dedupe. I would suggest to you that perhaps if you think you need it, you're making your stamps too big -- piecing together things that maybe shouldn't be pieced together in the first place? Maybe it's better to make smaller, independent stamps rather than merging together some large, complex stamp.
Let static analysis tools worry about that. =)
Again, if you're composing lots of stamps together, there's a good chance that maybe you're building a stamp that's too big.. doing too many things. Keep in mind the single responsibility principle, the principle of high cohesion, and "The Single Biggest Mistake Programmers Make Every Day". |
Just because you can merge lots of stamps into one giant god stamp doesn't mean you should. |
Hmm, I guess that makes sense, but it also makes one question true purpose of stamps. I mean if I went with small things, I could just write bunch of factory functions without anything extra to it. Stamps are essentially just adding bit of sugar syntax on specifying methods, properties, statics. |
Yes, indeed. I use them as sugar when I need to assign prototypes for fast property access (prototype property access triggers v8's fast object mode), as well as use private variables, privileged methods, and when I need to mix in a common APIs, such as event emitters. |
This is not the first time I'm having the following problem.
Stamp2 = compose(Stamp1, ...)
Stamp3 = compose(Stamp1, ...)
Stamp4 = compose(Stamp1, Stamp2, ...)
Stamp5 = compose(Stamp1, Stamp2, Stamp3, Stamp4, ...)
The problem: the expensive "init" is called five times. This is my production example.
Proposal: remove duplicate
init
functions from the.initializers[]
array.The change will touch this single line:
The lodash "uniq" function removes the same items form the back. In other words:
Any caveats I'm missing here?
The text was updated successfully, but these errors were encountered: