Skip to content

Fix type error cannot read property async #9

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

Conversation

AllenFang
Copy link
Member

Try to fix #7 (comment)

@AllenFang
Copy link
Member Author

@CYBAI, please review, seems like Travis was pass now.

@AllenFang AllenFang added this to the 0.1.0-alpha.1 milestone Jun 27, 2017
Copy link
Contributor

@CYBAI CYBAI left a comment

Choose a reason for hiding this comment

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

Thanks for your great work.
The fix looks good to me.
I think only the requiring path of utils in tests is the last part need updating.


import { FETCH_HEADER } from '../../utils/constants';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we require util without dots?
ex. import { FETCH_HEADER } from 'utils/constants'

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@AllenFang AllenFang force-pushed the fix-TypeError-Cannot-read-property-async branch from 6aae566 to 364e3b1 Compare July 11, 2017 13:04
@AllenFang
Copy link
Member Author

@CYBAI, I revert the lastest commit about resolving '../../utils/constants', Because for fixing this issue, I add following in package.json:

"moduleDirectories": [ 'node_modules', 'src-react/__test__' ]

But it most like cause a import naming conflict for app, for example,

You can imagine this line and guess what module will be imported?!.

I try to avoid it bug but still fails, and also I think it's not very important that must handle this relative path in testing code,

Please give some useful solution if you have, thanks

@AllenFang AllenFang merged commit 71e9349 into revamp-with-react:phase-1 Jul 11, 2017
@CYBAI
Copy link
Contributor

CYBAI commented Jul 11, 2017

@AllenFang Okay, if I have better solution for this, I'll discuss with you.

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

Successfully merging this pull request may close these issues.

2 participants