Skip to content

Conversation

Ilham-Habibullin
Copy link
Contributor

I think there is still a lot of work I have to do, now i want to get sure that i moving in right direction
@ffMathy you the one who I expect will assure me

@Ilham-Habibullin
Copy link
Contributor Author

@ffMathy I am planning to continue working on this PR after having your feedback

@ffMathy
Copy link
Owner

ffMathy commented Nov 11, 2019

Direction seems OK, if you can get the existing breaking tests passing.

Thank you so much for looking into this! <3

@Ilham-Habibullin
Copy link
Contributor Author

May be we should replace received method with some Symbol? That would free us from necessity of disabledFor stuff?

I will try without it, but just think about that.
Sequlize library uses this approach for operators in db queries, operators like $where.

…i was doing before was breaking test called "class method received"
…al method like "received" moved into Get unitlity, added method initialState.recordedGetPropertyState & initialState.recordedSetPropertyState
…at that expects same method of proxy. 2. Recording getState on call special method like "received" if disableFor was used on substituted object
@Ilham-Habibullin
Copy link
Contributor Author

@ffMathy I have made required fixes

"class method received" test was broken because I was throwing error in set of receivedProxy, now it does same thing as simple proxy of context

Two tests for disableFor also was broken, it actually seems like they was not working correctly, get, set and apply of disableProxy was sending its arguments object into get, set and apply of Context, which ones was expect not same list of arguments, for example, context.get was expecting only property, but it was getting target, this, property, and сonsequently getting target where it was expecting property

I fixed that

also it seems like received called with disableFor was never recording as getPropertyState inside InitialState, not sure if it was broken always or I did that, but anyway I fixed that as well

waiting for your feedback



@ffMathy
Copy link
Owner

ffMathy commented Nov 12, 2019

So all tests pass?

@ffMathy
Copy link
Owner

ffMathy commented Nov 12, 2019

If so, that's impressive!

@ffMathy
Copy link
Owner

ffMathy commented Nov 13, 2019

Do all tests pass?

@ffMathy
Copy link
Owner

ffMathy commented Nov 13, 2019

Or is it just the tests for issue #51?

@Ilham-Habibullin
Copy link
Contributor Author

All tests pass, 39 exact number, 1 tests skipped, but not by me

@ffMathy ffMathy merged commit 8ac9d68 into ffMathy:master Nov 13, 2019
@ffMathy
Copy link
Owner

ffMathy commented Nov 13, 2019

@IlhamKhabibullin the build fails on the build server.

spec/issues/59.test.ts(16,31): error TS2554: Expected 0 arguments, but got 1.

@Ilham-Habibullin
Copy link
Contributor Author

Ilham-Habibullin commented Nov 13, 2019 via email

@Ilham-Habibullin
Copy link
Contributor Author

Ilham-Habibullin commented Nov 13, 2019 via email

@Ilham-Habibullin
Copy link
Contributor Author

Ilham-Habibullin commented Nov 13, 2019 via email

@ffMathy
Copy link
Owner

ffMathy commented Nov 13, 2019

But didn't you mention that the tests were passing? How did you get it running on your machine?

@Ilham-Habibullin
Copy link
Contributor Author

Ilham-Habibullin commented Nov 13, 2019 via email

@ffMathy
Copy link
Owner

ffMathy commented Nov 13, 2019

Ah that makes sense then. Do you have the courage to try to fix the optional issue 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.

2 participants