PaulUithol / Backbone-relational

Get and set relations (one-to-one, one-to-many, many-to-one) for Backbone models
http://backbonerelational.org
MIT License
2.34k stars 330 forks source link

ES2015 Refactor #581

Closed bpatram closed 1 year ago

bpatram commented 6 years ago

This PR makes some fundamental changes to this library in how it exports itself and how some pieces work together. There is still some work to be done to further decouple some relationships, mainly for HasOne, HasMany, and Relation classes.

More information can be found at #572

bpatram commented 6 years ago

@PaulUithol @philfreo @DouweM I'm looking for any feedback (or in the very least a single LGTM response) 😄 this is a big step forward for future efforts to refactor, enhance and improve this project!

philfreo commented 6 years ago

I won't be able to closely review this, but +1 for doing an ES2015 refactor. I would just suggest trying to make the Changelog as dummy-proof as possible with respect to how to upgrade. Right now it could be a bit more explicit. Users need to change all their Model & Collection subclasses (if they have any relations) to inherit from the new ones, right? If they do this then is there any other change?

Thanks for all your hard work on this.

philfreo commented 6 years ago

If there are any specific commits, possible functionality changes, etc. that you'd especially like reviewed or are unsure about, please call them out specifically.

bpatram commented 6 years ago

@philfreo The main change (noticeable by end users) is how we export bbr since we are no longer modifying the Backbone namespace. There is an upgrade guide (https://github.com/PaulUithol/Backbone-relational/blob/next/UPGRADE_GUIDE.md) that details some of the changes with exporting for the major module systems (common js, es modules, node require statements). I can add a link to that guide in the changelog and in the final release notes.

I intend to make some actual improvements and bug fixes in future versions. I figured we wouldn't do too much in a single release considering the changes made on this branch.

philfreo commented 6 years ago

I see that you also added a CHANGELOG.md in next. Do we want to have that in addition to http://backbonerelational.org/#change-log and keep both up to date going forward, or stick with just one place? I just pushed a small fix to the website one.

Do you want to merge this soon and move forward on releasing the next version?

You don't see any reason to make a release from current master in between 0.10.0 and your next (0.11.0) do you? I see that Backbone 1.2.3 and then 1.3.3 support was added later after 0.10.0, but it seems like that didn't actually require any src changes right?

bpatram commented 6 years ago

@philfreo Sorry for my delays, I've been a bit busy with some other project work to keep this effort moving forward at a good pace.

I see that you also added a CHANGELOG.md in next. Do we want to have that in addition to http://backbonerelational.org/#change-log and keep both up to date going forward, or stick with just one place? I just pushed a small fix to the website one.

I think the website can omit the changelog or simply link to the CHANGELOG.md file in the repo if needed. I look to what the MarionetteJS team is doing as an inspiration and they seem to pull in the same markdown file (https://marionettejs.com/updates/). Maybe one day we can do the same for the documentation website here. For now I think omitting it (to remove redundancies) and using the CHANGELOG.md file would be sufficient.

Do you want to merge this soon and move forward on releasing the next version? You don't see any reason to make a release from current master in between 0.10.0 and your next (0.11.0) do you?

This is ready to be merged in now. I see no reason to hold it up or make a release inbetween 0.10.0 and (0.11.0).

I see that Backbone 1.2.3 and then 1.3.3 support was added later after 0.10.0, but it seems like that didn't actually require any src changes right?

Backbone 1.3.3 support was "added in" (really just tested against) but required no changes to code (aside from modifying unit tests) see commit https://github.com/PaulUithol/Backbone-relational/commit/c96ec7986aa21302e594c2e2810d2a5de43ebbbb

philfreo commented 6 years ago

All of the above SGTM 👍