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

Added garbadge collection for observables._expressions. FIxes #90 #94

Merged
merged 1 commit into from Oct 4, 2015

Conversation

Kanaye
Copy link
Collaborator

@Kanaye Kanaye commented Oct 3, 2015

Observables used in expressions will now get stored in
elementData.observables for garbage collection, before only observables in data-query methods got stored there.

ElementData.clear now also cleans up the javascript observables._expression and javascript observables._expressionKeys objects/arrays.
Fixes #90.

Also corrected a typo (Or at least i think it's a typo cause i could not find any case where data has an id property. Correct me if I'm wrong!)

Say if you don't like something with my solution or had something other with the elementData.observables array in mind.

Have a nice day.

Observables used in expressions will now get stored in
``elementData.observables`` before only data-query methods got stored
there.
Fixes astoilkov#90
@astoilkov
Copy link
Owner

Wow. This is great. This also fixes a problem in the todomvc demo which was preventing it to get merged into the master. Take a look here. Do you have the desire to push the todomvc app so it gets merged into the tastejs/master?

Regarding the solution: This is amazing. I was actually looking at the code the other day and also noticed the same think about data.id and have fixed it but didn't have the time to push it. You are doing amazing work. If you are interested in the future I could give you a push access to the repo.

One additional question. Do you run the tests before creating a pull request?

astoilkov added a commit that referenced this pull request Oct 4, 2015
Added garbadge collection for observables._expressions. FIxes #90
@astoilkov astoilkov merged commit aa6ef51 into astoilkov:master Oct 4, 2015
@Kanaye
Copy link
Collaborator Author

Kanaye commented Oct 4, 2015

Thanks for the kind words.

I can take a look (or a few more) at the todomvc app.

I will try to contribute in the future as I like jsblocks and it saved me time I can give back to it 😄 .

And for the last question: Yes I'm running the tests bevore creating a pull request.
I'm also testing a bit myself (e.g. for memoryleaks).
But currently I'm not writing new tests as I'm not familiar with jasmine.

@astoilkov
Copy link
Owner

I thought a little more about the todomvc app. I will handle it. It doesn't make sense you to spend time when I am familiar with it. I will handle it in the next couple of days.

Regarding the tests: If you want we could create a Skype call and introduce you to approaches how you could write tests and answer questions if you have any.

@Kanaye
Copy link
Collaborator Author

Kanaye commented Oct 4, 2015

Thanks for the offer. I know what to test or at least what I would test... But normaly I'm only resoponsible for serverside testing. I just have to look into jasmine.
I will ask questions the time I have some.
Regarding the push access to the repo: I would not mind to get it, but it's your decision. But in my opinion you should only give push acces to someone you trust, cause anybody (including me) can messup with git repositories.

Oh and I'm sorry my english 'sounds' sometimes a little bit funny. Feel free to ask if you have problems understanding me.

Have a nice day

@astoilkov
Copy link
Owner

Your english is perfect. Regarding the push access. I agree with you. I mean give you the access in the future when I see you are familiar with the code. From what I have seen until now are understanding it pretty well.

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.

None yet

2 participants