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

Windows symlink fix and typescript conversion #29

Closed
wants to merge 1 commit into from

Conversation

williamluke4
Copy link

I have not tested this on Mac or Linux

@adrianmcli
Copy link
Owner

@williamluke4 thanks for this! I'm going to test this out tomorrow and hopefully merge it soon.

@adrianmcli
Copy link
Owner

@williamluke4 actually, can I ask a favour of you? I'd like to consider the symlink fix and typescript integration separately. I think adding Typescript is going to be a bit too opinionated, but that's something we can discuss. But I don't want that to stop the symlink fix being merged.

@williamluke4
Copy link
Author

@adrianmcli Will do it when i have time. I don't really see any reason why people would be against typescript as there doesn't have to be any differences... It is just there if you want to have a better development experience

@williamluke4
Copy link
Author

@adrianmcli Have you had time to test it yet?

@adrianmcli
Copy link
Owner

@williamluke4 I don't think Typescript is appropriate for this project right now. And your symlink fix seems to be using a relative import that reaches outside of the client folder. This doesn't work on Mac OS X/Linux, which is why I used a symlink to begin with. In other words, we need to keep the symlink mechanism.

I'll be closing this PR for now, but don't hesitate to make an issue for discussion or submit another PR that you think I might accept given what I've stated above. Thanks for your effort!

@adrianmcli adrianmcli closed this May 24, 2018
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