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

#38 support windows development #114

Closed
wants to merge 3 commits into from
Closed

#38 support windows development #114

wants to merge 3 commits into from

Conversation

eloisetaylor5693
Copy link

#38 Added support for Windows development.

I didn't test every npm script.

Side note: I think the script should be called start instead of dev. eg yarn start or yarn serve:dev

@vercel
Copy link

vercel bot commented Mar 22, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/covid19-scenarios/covid19-scenarios/493cguk2a
✅ Preview: https://covid19-scenar-git-fork-eloisetaylor5693-38-support-wind-060f23.covid19-scenarios.now.sh

@eloisetaylor5693 eloisetaylor5693 changed the title #38 support windows dev #38 support windows development Mar 22, 2020
host: '0.0.0.0',
host: '127.0.0.1',
Copy link
Member

Choose a reason for hiding this comment

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

It will break for people who work on remote machines. Why are these necessary?
It works for me with 0.0.0.0 on Windows 7 VM.
If it is really necessary, please make it configurable through an environment variable and add to .env.example file

Copy link
Author

Choose a reason for hiding this comment

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

On both my windows machines, 0.0.0.0 didn't work. 127.0.0.1 is the standard localhost IP address.

Copy link
Member

Choose a reason for hiding this comment

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

@eloisetaylor5693 Could you please patch and verify it for us?
Add and .env variable and set it to 127.0.0.1 by default. Refer to this variable in webpack config. Those who need remote will change the var to 0.0.0.0,
Thanks for your investigation!

Comment on lines +12 to +13
"preprod": "node -e 'console.clear()' && yarn install && yarn prod:clean",
"prod": "cross-env NODE_ENV=production BABEL_ENV=production webpack --config config/webpack/webpack.client.babel.ts",
Copy link
Member

Choose a reason for hiding this comment

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

Using pre- hooks only makes things even more confusing in my already super-confusing setup. =)
Could we avoid that? As Microsoft folks explained in the issue thread, it is sufficient to just add spaces around semicolons ;.
In fact I was also working on it in parallel and it indeed works.

@ivan-aksamentov
Copy link
Member

ivan-aksamentov commented Mar 22, 2020

Hi @eloisetaylor5693

I've been working on this slowly in parallel too and I decided to crowdtest it here:
#38 (comment)

and in the end merged my changes, which I found more comprehensive and less breaking.
Here is a summary:
https://github.com/neherlab/covid19_scenarios/compare/de97b3b..28b1468

I will close this for now. But if you still need to adjust something, feel free to jump in to #38 or in the chat or to make another PR

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