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

Some questions and notes #68

Open
kuraga opened this issue Jan 17, 2016 · 2 comments
Open

Some questions and notes #68

kuraga opened this issue Jan 17, 2016 · 2 comments

Comments

@kuraga
Copy link
Contributor

kuraga commented Jan 17, 2016

1) Tag 1c17fe3 as v0.9.4, please.

  1. Propose to use ===/!== instead of ==/!=.

  2. Same for tests: strictEqual/notStrictEqual instead of equal/notEqual.

  3. Is this assert.deepEqual(data, example); instead?

  4. What does remove returns being called with unexistant key(s)? Is it fixed in tests?

  5. Listener tests: I don't see tests about triggering callbacks on exact .remove call.

  6. Listener tests: I don't see tests about .trigger function.

  7. Listener tests: "Listen by several callbacks" test is omitted.

  8. Listener tests: Will callback triggered if we try to remove non-existant property?

  9. Listener tests: Will callback triggered if we try to update a property but with the same value?

  10. Listener tests: ".unsubscribe doesn't touch other callbacks" test is omitted.

  11. Listener tests: What's the behavior if we pass non-existant callback to .unsubscribe?

  12. Could we merge (some parts of) freezer-spec and hash-spec?

@arqex , thanks! (Note: I've read freezer-spec, hash-spec and listeners-spec only. I use them here.)

@arqex
Copy link
Owner

arqex commented Jan 18, 2016

Thanks for your list @kuraga

  1. I skipped that buggy version in github since I have released the 0.9.5.
  2. I merged your PR for this.
    3 & 4) I had problems with strictEquals in some tests made for the browser that are outdated now. It would be great to make the test pass again in browser so maybe should be nice to use some assertion library that can be use there too.
  3. remove should return always the object where the attributes must be taken off. If the key is not there nothing is removed and no error is raised, but the node where the remove was called is replaced in freezer by a new object. This behaviour is different of what we have in set, there is no change happens freezer's state is not modified at all. Maybe we should modify this remove behavior to match set one.
    6-12) Testing and testing.... nobody like testing, I am human and I don't like it either 💃 . But it is true that if we have a wider test suite we could make bigger changes in the library being confident nothing will get broken.
  4. I merge some of your tests, and I hope I can merge others soon.

By the way, rotor.js!!! What a nice name for a new framework!! I'd like to see it working, do you have some demo online??

Cheers,
Javi

@kuraga
Copy link
Contributor Author

kuraga commented Jan 18, 2016

@arqex big thanks for reply!

2-4) Note problem is wider: not only in pointed places (you know it 😄).
5) Cool! Document and/or change.
6-12) Ok. I've made all of these tests but I use other test and code style.

  1. I meant another one. Seems like freezer-spec and hash-spec have some common tests. But, maybe, they are different... For root of freezer and nested object... Hm...

Coolness of RotorJS is its modularity. Some of FreezerJs tests are like a standard but you may use other library (I think even mutable) for model (FreezerJs Model file).

The same for other components such as application loop. You may use (virtual) DOM library or make Canvas support (now I support virtual-dom only).

There is an example. I'm creating online demo (another maybe, I think ToDoMVC after stabilizing of API. But coolness is code not possibilities (they are minimal).

Big thanks! Feel free to close.

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

No branches or pull requests

2 participants