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

MobX, MST, TypeScript, Jest, TSLint upgrade (#2811, #2971) #2995

Merged
merged 19 commits into from
Oct 26, 2018
Merged

Conversation

aksonov
Copy link
Contributor

@aksonov aksonov commented Oct 24, 2018

No description provided.

@aksonov
Copy link
Contributor Author

aksonov commented Oct 24, 2018

@southerneer Feel free to continue upgrade. I upgraded all(?) typescript-related libraries including ts-jest, but now jest tests fail with 'Unexpected token import'

@aksonov
Copy link
Contributor Author

aksonov commented Oct 24, 2018

UPDATE:

I've fixed jest import error by upgrade of jest/babel-jest and disabling of useBabelRc. Now some jest tests fail because of more strict MST3 - it doesn't allow set real instances instead of references as we usually do for unit tests (and probably somewhere else, have to test the app)...

I changed homeStore.test.ts to pass references correctly (i.e. add bot to some test model, so references could work), but the solution looks overcomplicated. Feel free to use simpler method if you can.

@aksonov aksonov changed the title WIP: mobx, mst upgrade (#2811) WIP: MobX, MST, TypeScript, Jest, TSLint upgrade (#2811, #2971) Oct 24, 2018
@southerneer
Copy link
Contributor

southerneer commented Oct 25, 2018

I started down the path of converting Jest snapshot tests from js -> tsx and eventually realized that mockWocky is a mess and needs to rewritten (or even re-thought). Since the main goal of this PR is to get MST, TS, tslint, etc upgraded I'm going to create a separate ticket for fixing Jest snapshot tests so we can review and merge this one (as it's already a pretty big batch of changes).

@southerneer
Copy link
Contributor

southerneer commented Oct 25, 2018

testLocal passes now but I'm seeing MST-related errors in mochaWocky tests:

Error: [mobx-state-tree] Failed to resolve reference '1e581254-d7fc-11e8-a4da-17f3c19eb4be' to type 'Bot' (from node: /result/0)

Any ideas?

@aksonov
Copy link
Contributor Author

aksonov commented Oct 25, 2018

Unit tests fail for the same reason as homeStore - MST 2 allows to store real instances as references, but MST 3 - doesn't (so 'detached' paging results works fine with MST2 but not with MST3).

My future attempts to solve this: #3003. I decide to make it as separate PR because my attempt ends with big mess with typescript, many typings just lost (Profile/Bot), probably because of circular references:
mobxjs/mobx-state-tree#417

So probably we should stay with MST 2 for a while..

const data = {...param}
// some workaround to create references on the fly (maybe recent MST can do it automatically?)
if (param.user && typeof param.user === 'object') {
// create reference to profile!
self.profiles.get(param.user.id, param.user)
data.user = param.user.id
}
if (param.profile && typeof param.profile === 'object') {
// create reference to profile!
Copy link
Contributor

Choose a reason for hiding this comment

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

This "manual" switching from object to MST ref seems really dirty. Once we switch to all GraphQL we can use __typename to manage this with a little more flexibility but even still that seems like a hack. It seems like there should be a better way to dynamically inspect an incoming object for MST references.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is dirty, I agree, but I don't see better option

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #3011
Once we have some idea, we can do it.

@aksonov aksonov changed the title WIP: MobX, MST, TypeScript, Jest, TSLint upgrade (#2811, #2971) MobX, MST, TypeScript, Jest, TSLint upgrade (#2811, #2971) Oct 26, 2018
@aksonov
Copy link
Contributor Author

aksonov commented Oct 26, 2018

@southerneer Merging, feel free to review

@aksonov aksonov merged commit 247e9b3 into master Oct 26, 2018
@@ -8,6 +8,7 @@ import {Timeable} from './Timeable'
import {Profile} from './Profile'
// import {ProfileRef} from './Profile'

const BotPostProfileRef = types.late('LazyProfileRef', (): IAnyModelType => Profile)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

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