cytoscape / cytoscape.js

Graph theory (network) library for visualisation and analysis
https://js.cytoscape.org
MIT License
10.09k stars 1.64k forks source link

Feature request: ES6 iterators protocol support in cy.add() #2603

Closed nicky1038 closed 4 years ago

nicky1038 commented 4 years ago

Currently cy.add method works with cy element collections and with element definition arrays.

It would be cool to make this method work with element definition iterators as well to get users rid from obligatory caching of all element definitions into temporary array before adding them to the graph.

It can also be handy for users to be able to work with iterators of single cy elements and asynchronous iterators.

maxkfranz commented 4 years ago

I think iterable support is more important for consumption (i.e. using collections) rather than production (i.e. creating elements). Collections themselves are already iterable. You can for of a collection.

I'm not generally against supporting iterables for production, but I can't think of a use case off hand:

It would be nice to have a specific use case for this, but on the other hand, compatibility with the latest ES features is a good general goal. To add this feature, we'd have to consider any other places in the API that would similarly accept iterable parameters, for consistency's sake.

On Fri, Jan 10, 2020 at 12:09 PM nicky1038 notifications@github.com wrote:

Currently cy.add method works with cy element collections and with element definition arrays.

It would be cool to make this method work with element definition iterators as well to get users rid from obligatory caching of all element definitions into temporary array before adding them to the graph.

It can also be handy for users to be able to work with iterators of single cy elements and asynchronous iterators.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/cytoscape/cytoscape.js/issues/2603?email_source=notifications&email_token=AAHRO43S57FZVVVD7RCAM6LQ5CTWHA5CNFSM4KFKX6D2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4IFMYNJQ, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHRO4ZUHK7NPYUYQZXUNHLQ5CTWHANCNFSM4KFKX6DQ .

nicky1038 commented 4 years ago

When fetching data from the server and deserialising via JSON.parse(), the elements would already be an array. <...> It would be nice to have a specific use case for this

The main point is that this fetched data is not passed straight into cy.add. First it needs to be transformed into node definition objects, and for now these definition objects should only be stored into a new temporary array before passing as an argument. What means extra memory allocation. This allocation could be avoided if one uses [Symbol.iterator]() on data array, obtains an iterator of the transformed data (without iteration itself) and passes it into cy.add. If only cytoscape.js supported this.

You can already call cy.add() for one element, without any array

Using cy.add on each single element definition object inside for..of loop can do the trick, but it could be better for users if this iteration was performed by cytoscape. This could slightly reduce the amount of user code and allow to focus on what to do (pass element definitions) instead of how to do it (iterate over them and add one by one).

So passing an iterable of promises doesn't make sense to me

I in turn agree that async iterables support is way too questionable and may not be considered.

To add this feature, we'd have to consider any other places in the API that would similarly accept iterable parameters

A cursory glance on the list of all API methods showed cy.add is unique of its kind.

maxkfranz commented 4 years ago

I agree that it could be a useful feature for the synchronous case. If you'd like to make a pull request for this feature, that would be great.

Note that the implementation within the library won't be able to rely on Babel. It would have to rely on feature testing and manually using the iterator, as this library has support for legacy browsers. The iterator support would work only for modern browsers or with polyfills.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale, because it has not had activity within the past 30 days. It will be closed if no further activity occurs within the next 30 days. If a feature request is important to you, please consider making a pull request. Thank you for your contributions.

umdstu commented 4 years ago

@nicky1038 I would perhaps suggest you consider using rxjs subscriptions on your calls for the json data along with pipe, map, mergeMap, etc. This will let you pre-process the data as it comes in and before it gets to cy.add();