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

Use immutable data structures #9

Closed
wants to merge 2 commits into from
Closed

Conversation

jorinvo
Copy link

@jorinvo jorinvo commented Jun 13, 2015

Thank you for sharing the concept of Cycle.js!
The architecture makes things really clear and simple.
This is my new favorite TodoMVC implementation 😉

I was playing around with immutable.js and saw the issue #4 later. Thought I might send a pull request.
Feel free to modify this, if I missed something. I'm pretty new to this technologies, so I probably don't use it all the right way yet..

jorinvo added 2 commits June 13, 2015 16:59
- include sourcemaps when built with npm run build-debug
- run browserify and uglify with pipe
id: todoData.id
})
)
list: todosData.get('list').filter(x => x ).toJS()
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes I got null values in the list. Don't know if this was a testing issue or something about immutable.js I haven't discovered yet..
That's why I filter the values here ...

@staltz
Copy link
Member

staltz commented Jun 15, 2015

Hi @jorin-vogel, thank you for this. I really like Immutable.js and I would use it myself in any project. But for TodoMVC itself, I'm afraid we have to keep it out because in order to compare with other frameworks, it's best if the TodoMVC implementation is "vanilla Cycle.js". Issue #4 was just a suggestion of how to speed up performance, it wasn't necessarily about Immutable.js (the name betrays, but there is a difference between "immutable" and "immutable.js").

@jorinvo
Copy link
Author

jorinvo commented Jun 16, 2015

@staltz ok cool. I totally get this.
I was just playing around with immutable.js. But I also felt like it would be overkill for TodoMVC.
If anyone asks how to integrate immutable.js though, we have an example now to show them 😃

@jorinvo jorinvo closed this Jun 16, 2015
@staltz
Copy link
Member

staltz commented Jun 16, 2015

And to not completely lose the effort you put into this, I put a link to this PR on the README.

@jorinvo
Copy link
Author

jorinvo commented Jun 16, 2015

Thanks, that's awesome! Let me know if I can improve anything.

@donaldpipowitch
Copy link

Thank you both. If anything can be improved, I'd be interested in a more complete version, too.

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

Successfully merging this pull request may close these issues.

4 participants