jayphelps / core-decorators

Library of stage-0 JavaScript decorators (aka ES2016/ES7 decorators but not accurate) inspired by languages that come with built-ins like @​override, @​deprecate, @​autobind, @​mixin and more. Popular with React/Angular, but is framework agnostic.
MIT License
4.51k stars 263 forks source link

add @once - forces a method to run only one time #99

Closed zivl closed 7 years ago

zivl commented 7 years ago

After first run, the return value is saved and for every other run of the method it will return the first result right away

jayphelps commented 7 years ago

Thanks for the PR!

I'm hesitant to add this decorator because I'm not confident it will see much usage--I don't want core-decorators to include every decorator, especially those which overlap lodash. If we maintain our own, that means double the work for not much benefit.

e.g. you can use the @decorate decorator to apply most lodash utilities

class Foo {
  @decorate(_.once)
  runSumOnlyOnce(num1, num2) {
    return num1 + num2;
  }
}

There are some existing decorators that overlap with lodash, but mostly I felt they were more likely to see use or they were added prior to the addition of @decorate.

I'm open to discussion of course! 😄 Thoughts?

zivl commented 7 years ago

@jayphelps - actually I haven't notice lodash has once function... yet, I agree, no need to re-write lodash in decorators. Never the less, I think once capability should be part of core-decorators project. Also, thinking out loud, looks like with ES7 - all lodash utilities that are related to functions - should be transformed into decorators - this is how I see it.

jayphelps commented 7 years ago

@zivl

Also, thinking out loud, looks like with ES7 - all lodash utilities that are related to functions - should be transformed into decorators - this is how I see it.

To clarify, decorators aren't in ES7 aka ES2016 (they aren't in any finalized spec yet) but I totally get your sentiment. I really don't want to reinvent them because we'll have to deal with the same bugs they worked through. Instead, if someone wants to, I think a library could simply import lodash and provide decorator "wrapper" versions of them that just defer to the lodash implementations.

I'm not sure core-decorators is the place for that--maybe it is. I'd be interested to hear what the community thinks. Not all of lodash makes sense as decorator, only the factory stuff, so it might be hard to clarify to people.

jayphelps commented 7 years ago

Going to keep this open for a couple weeks to see if someone has further thoughts on lodash duplication or some sort of lodash wrapper library.

nrdobie commented 7 years ago

I think it would make sense to create another package like lodash-decorators. It would be possible to have the package use the @decorate from core-decorators and import the standalone Lodash functions, e.g. lodash.once. Going one step further, the package could try to use the Lodash version installed by the user via optionalDependency to help reduce bloat.

jayphelps commented 7 years ago

@nrdobie that's very close to my thinking as well. I imagined you'd need to take a similar approach to lodash--providing ESM bundles so they can use Rollup/Webpack2 to tree shake.


Actually, this already exists https://www.npmjs.com/package/lodash-decorators (doesn't use core-decorators cause it really doesn't need to)

Seems like this is a solved problem?

jayphelps commented 7 years ago

Now that https://www.npmjs.com/package/lodash-decorators exists, I'm going to think about deprecating then removing all our decorators that overlap with it and keep core-decorators only to more fundamental ones like @readonly and @autobind

Of course, linking to https://www.npmjs.com/package/lodash-decorators as well 😄

zivl commented 7 years ago

Hey, I think that a library needs to evolve. Over time, libraries that are not evolving are eventually deprecated due to out-of-date relevance. So sure, there are overlaps here and there, but it is up to the consumer wether to use both libraries or only one of them (or in the lodash case, can use only a single or a few functions).

let me know what's the final decision so I will resolve conflicts in order to merge

jayphelps commented 7 years ago

@zivl I'm confused. Certainly libraries need to evolve. Why would core-decorators duplicate the effort of lodash-decorators? If not doing so ends up meaning that core-decorators is no longer relevant, I think that'd be a good thing--but I strongly disagree. They have different goals.

I'm pretty happy with deprecating then removing decorators that overlap with lodash.

I really do appreciate your work on this PR and the discussion, but I don't want to duplicate effort. We could never keep up with the robustness of lodash.

jayphelps commented 7 years ago

I've made a initial PR to kick start the deprecation process of the overlapping decorators: #110 feel free to comment!

zivl commented 7 years ago

yeah I got your point... I guess you're right. anyway, I'm happy we could discuss that issue and come to that conclusion.

Cheers!

jayphelps commented 7 years ago

Thanks for understanding! 🎉

jayphelps commented 7 years ago

We've officially deprecated all the decorators that overlap with lodash-decorators, so this one won't make it. 😢 Thank you for the work you put into this!