Reorders 'saving' and 'creating' triggers as per docs.

[davestevens]

The docs suggest the ordering of triggers on the Model are:

• Saving
• Creating
• Updating
• Created
• Updated
• Saved

see: https://github.com/tgriesser/bookshelf/blob/master/docs/src_model.js.html#L951-L956

Currently Saving is triggered after Creating/Updating. I am using Saving to hook in a validation of the model, and I want to validate it before saving (for example validate that a password and password confirmation are the same, but then delete the password confirmation as I don't want to store it in the database).

[vellotis]

@davestevens I have to say that I agree withe the suggested change as save gets called before create or update. Actually all the other ORMs that are somehow mimic ActiveRecord behave that way.

@rhys-vdw I know this may be a vital change for someone. But the documentation denotes this behaviour.

[kirrg001]

@ricardograca I don't understand why this got released in a minor. This is a breaking change. In my opinion this change belongs into a major release 😒 (1.0.0)

Edit: Just to be clear. I agree that this was (maybe) a wrong implementation from the beginning, but if you have a big application which relied on this code since it was added, it's breaking your code. I am saying "maybe", because i am not sure that this order makes really sense.

I assume it will break every code, which makes use of creating and saving.

I think this commit needs a revert + a re-release

[ricardograca]

Well, I'm following semver here that states that:

Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable.

and

Major version X (X.y.z | X > 0) MUST be incremented if any backwards incompatible changes are introduced to the public API.

Here X is 0 so this doesn't apply.

I don't think this should be reverted, because it would just be postponing the problem, and version 0.x is precisely to test things and to potentially break stuff. My idea is to try to get to a point where a major 1.0.0 version will be stable for as long as possible, so some breakage will occur before that.

Do you have any real world examples of this breaking any code or is it more a theoretical problem?

[kirrg001]

Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable.

Bookshelf is not in the state of initial development. Bookshelf missed the point to release a 1.0.0 to be able to then follow the semver restrictions (breaking changes in major versions). We are using bookshelf for > 3-4 years.

Do you have any real world examples of this breaking any code or is it more a theoretical problem?

If you are using a custom id value for example. You relied on the fact that creating is triggered first - you generate the id. And the saving event relies on the fact that the id is present. There are for sure lot's of cases. This is just a simple one.

Edit: If you are validating your model against a schema. You relied on the fact that creating was triggered first, which does general things like checking for dynamic defaults, removing things etc. and then saving is triggered, which validates the schema against a model which was pre-checked.

---

I don't think this should be reverted, because it would just be postponing the problem

I don't really agree. You can decide (the user who uses bookshelf) if you want to upgrade to the next major and schedule time to correct breaking changes.

[ricardograca]

Bookshelf is not in the state of initial development. (...) We are using bookshelf for > 3-4 years.

That's debatable. There have been some breaking changes over the years, and we never released a 1.0.0. Time is not what determines if something is stable or in development.

Your example makes sense, but is it a real example or a hypothetical one? Because unless someone comes up and actually complaints about this change breaking actual programs and doing the necessary changes to their code base not being a viable option because it would be too daunting of a task and they really need some change that was introduced with 0.13.0 then I don't see a reason to revert this.

If someone is worried about this change breaking something they can always stay on version 0.12.1. If the problem is not being able to update code then why would you even update a dependency?

However I also don't want to purposely cause anyone harm, so if you can provide a valid reason that is not based on theoretical problems then we can always backport the other non-breaking changes to the 0.12.x branch.

[kirrg001]

Your example makes sense, but is it a real example or a hypothetical one?

Both examples i've shared are real cases. The release 0.13 breaks Ghost. Ghost makes heavy usage of the events which are triggered from Bookshelf. Now that the event order is different, we would have to rewrite the actions which are executed in the event handlers. Another real example is bookshelf-relations, which is also based on events and expects creating being triggered before saving.

I am not saying we have to revert this commit only because of Ghost, but Ghost is i think one of the biggest platform using bookshelf and i think we have to take care shipping breaking changes for a project which is in my opinion, as said in my previous comment, not in the initial development state anymore.

Also from https://semver.org/:

How do I know when to release 1.0.0?
If your software is being used in production, it should probably already be 1.0.0. If you have a stable API on which users have come to depend, you should be 1.0.0. If you’re worrying a lot about backwards compatibility, you should probably already be 1.0.0.

Ghost is using bookshelf on production for more than 3 years.

If bookshelf continues with sticking with 0.x, people have to adapt the breaking changes, otherwise they won't receive important bug fixes, which is now the case. And breaking changes like changing the order of events in an ORM, is nothing you can fix within a minute in a bigger application.

[ricardograca]

Ok, those are valid examples. The thing is that the order of events will change eventually, so what difference does it make if it's now or 3 months from now? You will have to adapt your code base anyway right? That said I don't mind postponing this change until the next release, if that is a good compromise for you, but it has to be released at some point.

Also, there are other breaking changes in this release, including your PR about running async events in sync. It probably doesn't affect as many people but it doesn't change the fact that it is a breaking change, so should it also be removed? Breaking changes will occur and I would rather not pile them up all for the future 1.0.0 release.

As for the current version names I agree that Bookshelf should have released version 1.0.0 long ago, but that hasn't happened yet, so I'm trying to work towards it. In the mean time there will be some breakage.

However I don't think your problem is the version number, since if we were already at a major version this would be the next major version and everything else would be the same and you would still be complaining about the order of events.

In the future would it help if I do some pre-releases before the final versions? Would that give you enough time to prepare for any potential changes?

[ricardograca]

BTW, I will be unable to do a new release in the next couple of hours, but if you make the necessary changes you can merge them and I'll do a release as soon as I get the chance.

Also, this will be a 0.13.1 release, which breaks semver, but I don't think anyone has built an application yet depending on the order of events of version 0.13.0.

[kirrg001]

Ok, those are valid examples. The thing is that the order of events will change eventually, so what difference does it make if it's now or 3 months from now?

The difference is that if we would have released a proper 1.x version (not saying this is your fault 😎) and i want to stay on 0.x - Bookshelf should back port important bug fixes to 0.x. So if i don't want to spend time now to adapt breaking changes, i don't have to.

I would suggest to keep this as is and release a 1.x asap 👍 I've refactored Ghost till now and it looks like everything should work.

[ricardograca]

Good points, but I'm working for free and practically alone on this, so there's just so much I can do to support older branches and backport important fixes. However I created a 0.12 branch a while back precisely to be able to do this if needed. If someone else complains about one of the breaking changes I'll look into making a point release in the 0.12 branch.

I'm glad you got it sorted. I'll keep you posted on important breaking changes in the future.

I would suggest to keep this as is and release a 1.x asap

I'm working on it! 😅