Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

Remove nestedset dependency #148

Closed
JN-Jones opened this issue May 21, 2015 · 9 comments · May be fixed by #150
Closed

Remove nestedset dependency #148

JN-Jones opened this issue May 21, 2015 · 9 comments · May be fixed by #150
Assignees
Milestone

Comments

@JN-Jones
Copy link
Contributor

The only part of it we're using is the node functionality (parent_id) which can easily be added ourself. To remove some overhead we should remove the library and write our own trait instead.

@JN-Jones JN-Jones self-assigned this May 21, 2015
@JN-Jones JN-Jones added this to the Alpha 1 milestone May 21, 2015
@wpillar
Copy link
Contributor

wpillar commented May 21, 2015

Important to consider this one, whilst removing the 3rd party dependency will reduce final package size and a little bit of build time, implementing something ourselves has the overhead of maintenance in the long-term, writing ourselves is not overhead-free.

So my question is this, is the saving of overhead from removing the 3rd party dep more than the overheard we'd be generating by rolling our own? Personally, I'd rather have extra file size and build time than maintaining more code ourselves.

@JN-Jones
Copy link
Contributor Author

Well, nestedset has:

  • Support for linear trees (parent_id)
  • Support for left/right id
  • Thousands of functions to ensure a tree is really a tree, to fix trees, to build trees etc pp

What we need:

  • A forum has a parent id

We're calling one or two functions:

  • get tree (where parent_id is null)
  • parent relation

Both are one liner functions and one class variable.

@JN-Jones
Copy link
Contributor Author

On a side note: this change would also allow us to make use of the AbstractCachingModel class to remove some (a lot) of the queries on the index.

@euantorano
Copy link
Member

Yep, the library is pretty much useless here, and using our own code (like one method) brings a whole lot more benefits.

@wpillar
Copy link
Contributor

wpillar commented May 22, 2015

Cool 👍

@Matslom
Copy link
Contributor

Matslom commented Nov 28, 2016

The only part of it we're using is the node functionality (parent_id) which can easily be added ourself.

We are using only this one feature?
If we will remove left and right id:

  1. How we will handle forum order?
  2. How to handle forums in forum in category
  • category
    --- forum
    ----- subforum
    ----- subforum
    ----- subforum

Also we need some stuff to create, order or remove forums from ACP.

It is true that nestedset dependency have more features than we need but there are some ready to use. We can make our own trait using some methods from this dependency (MIT license).

@euantorano
Copy link
Member

At the minute, those are the only features we are using. Ordering can be solved with an order INT DEFAULT 0 column.

Nested sets has some nice features, but from what I remember it doesn't generate very nice queries (this might have changed - we are using a very old version).

@Matslom
Copy link
Contributor

Matslom commented Nov 28, 2016

Yep, for that moment the queries are ugly.
There are planned subforums like in 1x, right? So anyway we must add two columns: one for order, second for flag forums as parent (for subforums feature).

Ok then, I will remove this package and take up this ugly queries.

@euantorano
Copy link
Member

euantorano commented Nov 28, 2016 via email

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants