search

stampit-org / stampit

OOP is better with stamps: Composable object factories.
https://stampit.js.org
MIT License
3.02k stars 103 forks source link

Stampit v4 #326

Closed koresar closed 7 years ago

koresar commented 7 years ago

Closes #323 #317 #304 #68

Changes:

In case of silence I'll merge this PR in a week.

danielkcz commented 7 years ago

Phew, sorry @koresar, but I don't have a space to review this bulk of changes.

Honestly, I am not even sure why to bother with another stampit release when there is stamp monorepo. Why not to just have a stampit@v4 to reference @stamp/it instead? Why two different code bases?

koresar commented 7 years ago
  1. Stampit will become compatible with monorepo. Some were complaining about it.
  2. Size reduce. Good for bragging and websites perf.

The other changes have just happened along the way.

On 22 Oct. 2017 23:53, "Daniel K." notifications@github.com wrote:

Phew, sorry @koresar https://github.com/koresar, but I don't have a space to review this bulk of changes.

Honestly, I am not even sure why to bother with another stampit release when there is stamp monorepo. Why not to just have a stampit@v4 to reference @stamp/it instead? Why two different code bases?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stampit-org/stampit/pull/326#issuecomment-338475126, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjCL1VhB8o_RCjiLOkXiwJxQ5r-re1Pks5suzrHgaJpZM4QBhZU .

danielkcz commented 7 years ago

Sorry, but that still does not explain why to keep two almost identical implementations. When I started the idea about monorepo I thought that eventually, stampit module will be just a redirect to this monorepo and after a while it might be even deprecated and removed.

Right now it's kinda unclear where is the difference. Is it possible to use extra modules from the monorepo with stampit? That's an example of a user question, I know they are both up to spec including composers, but in my opinion, it just makes things worse.

I can imagine it wasn't easy to accomplish size reduction you wanted, but it feels kinda OCD :) Do you know someone who actually complained that this lib is couple bytes larger than it could be? I am probably living in a different world where a size of the libs is so much higher and it does not really matter in overall.

Personally, I have kinda lost interest in stamps in overall. Perhaps I've never understood it that well to use it efficiently and without losing grasp over my codebase. In complex scenarios, it was just too messy for my taste. Not even mentioning adoption when working on the team and explaining everyone how it works.

If you feel like you want to go this direction then go ahead and merge and release it. I am not best to judge this anymore. Sorry friend.

koresar commented 7 years ago

Stampit have to jump to next major version only because it is not following the latest spec v1.5. If we would vote to make "composers" part of the spec year ago stampit would have stayed v3. Unfortunately, community took a careful decision to do baby steps - implement "composers" as an experimental feature first. So, stampit v4 is a necessary last baby step towards compatibility with the latest specification v1.5.

AlexxNica commented 7 years ago

@koresar Little tip: You could have rebased your commits before pushing and merging to avoid polluting the history.

Take a look at: https://git-scm.com/docs/git-rebase https://git-scm.com/book/en/v2/Git-Branching-Rebasing

koresar commented 7 years ago

Totally forgot to squash before merging. My bad.

Licence was removed by accident. Will readd, or feel free to pr. Sorry about that.

On 15 Nov. 2017 15:50, "Alexandre Nicastro" notifications@github.com wrote:

@koresar https://github.com/koresar Little tip: You could have rebased your commits before pushing and merging to avoid polluting the history.

Take a look at: https://git-scm.com/docs/git-rebase https://git-scm.com/book/en/v2/Git-Branching-Rebasing

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stampit-org/stampit/pull/326#issuecomment-344485055, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjCL6gmxfGJuKqjrvtwbElK8jllvpAEks5s2m2FgaJpZM4QBhZU .