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

Fix scripts for Windows environment #118

Closed

Conversation

mickeypearce
Copy link
Contributor

@mickeypearce mickeypearce commented Mar 22, 2020

Description

Fixing package.json scripts for Win environment

Related issues

Impacted Areas in the application

Testing

Deploy Notes

@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/nvk1nof3g
✅ Preview: https://covid19-scenarios-git-fork-mickeypearce-windows-dev-scripts.covid19-scenarios.now.sh

@mickeypearce
Copy link
Contributor Author

mickeypearce commented Mar 22, 2020

I just realized now that there was already an opened pull request for this issue (#114) a few hours ago.
@eloisetaylor5693 Sorry my bad. 🥈

@ivan-aksamentov
Copy link
Member

Hi @mickeypearce

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

Comment on lines +15 to +16
"lint": "yarn eslint && yarn stylelint && yarn tsc && yarn tslint",
"lint:fix": "yarn eslint:fix && yarn tsc && yarn tslint:fix",
Copy link
Member

Choose a reason for hiding this comment

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

This is not the same behavior. This will stop after the first command that errors out.
I'd prefer to see all the errors, in both dev and CI environment.

I think we could just add spaces around semicolons. That should unscrew Windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @ivan-aksamentov ,

the line:
node -e 'console.clear()' ; yarn dev:clean ; yarn install && yarn dev:nowatch || cd .
doesn't properly work for me on Windows (neither with Powershell nor default CMD shell, maybe with a bash shell?)

it actually runs just the part after && (yarn dev:nowatch, etc) and skips the part before (clear, dev:clean, install).

yes, it properly runs the server etc but ';' doesn't seem to work properly on Win (at least for me).

Copy link
Member

Choose a reason for hiding this comment

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

If you want, you can play with
https://www.npmjs.com/package/npm-run-all

in particular, run-s with -c flag when appropriate
https://github.com/mysticatea/npm-run-all/blob/HEAD/docs/run-s.md

Let's hope it won't cause too many troubles on Unix.

Copy link
Contributor Author

@mickeypearce mickeypearce Mar 24, 2020

Choose a reason for hiding this comment

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

@ivan-aksamentov
It simplifies "lint" scripts quite a bit:

"lint": "run-s -c eslint stylelint tsc tslint",    
"lint:fix": "run-s -c eslint:fix tsc tslint:fix",

but clutters a bit more "dev" ("prod") scripts as

"exec": "run-s -c clear dev:clean dev:start || cd ." in dev.json

needs definitely adding "dev:start" and even "clear" to package.json:
"dev:start": "yarn install && yarn dev:nowatch",

Maybe it is a bit cleaner this way, on the other hand this exec line could be run even sequentially with exit on error:
"exec": "node -e 'console.clear()' && yarn dev:clean && yarn install && yarn dev:nowatch || cd ."
to compromise complexity.

Let me know If I should open a PR. thanks.

Copy link
Member

Choose a reason for hiding this comment

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

@mickeypearce Mickey, absolutely, open a PR! And thanks for the investigation.

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.

Support developing on Windows
2 participants