Skip to content
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

ES2015 Refactor #581

Closed
wants to merge 24 commits into from
Closed

ES2015 Refactor #581

wants to merge 24 commits into from

Conversation

bpatram
Copy link
Collaborator

@bpatram bpatram commented Nov 26, 2017

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 and others added 22 commits July 26, 2016 08:26
- semaphore prototype
- blocking queue
- event queue
- configuration settings

create new base object used to extend off of. relation and store uses this now.
… builds minified version too now.

tweaked linter rules for spacing and console output.
moved collection into its own module.
refactor some pieces of code to fit linter rules.
…relation, hasone, and hasmany.

moved store and relation into their own module
…on now.

updated rollup and babel (and presets)
finished separation of modules into their own files for the most part
updated build tools.
updated qunit to latest 2.x version.
added code coverage reporting.
unit testing uses source files instead of distributable.
refactor unit tests to be more modular.
updated readme to include test badge and organized info
disable travis email notifications
update codebase to match new linter styles
update tests to match new linter styles.
allow relatedModel to be defined as a function again.
fix some code style issues.
rebuild distributable
@bpatram bpatram added this to the v0.11.0 milestone Nov 26, 2017
@bpatram
Copy link
Collaborator Author

bpatram commented Dec 9, 2017

@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
Copy link
Collaborator

philfreo commented Dec 9, 2017

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
Copy link
Collaborator

philfreo commented Dec 9, 2017

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
Copy link
Collaborator Author

bpatram commented Feb 20, 2018

@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
Copy link
Collaborator

philfreo commented Mar 27, 2018

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
Copy link
Collaborator Author

bpatram commented Mar 29, 2018

@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 c96ec79

@philfreo
Copy link
Collaborator

philfreo commented Apr 2, 2018

All of the above SGTM 👍

bpatram and others added 2 commits February 8, 2019 23:59
* Add an option to run the tests with lodash instead of underscore

* Make all tests pass with both Underscore and Lodash 4 (fix #561)

* Add the lodash variant of the test suite to the Travis build

* Update the documentation to reflect that Lodash >= 3 works

* Mention the --lodash option in the CONTRIBUTING.md

* simplify lodash test command

* rebuild lib
@philfreo philfreo removed their request for review July 3, 2019 12:32
@bpatram bpatram closed this Jan 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants