Skip to content
This repository has been archived by the owner on Jan 6, 2021. It is now read-only.

Closes #153 with documentation update #155

Closed
wants to merge 1 commit into from

Conversation

achingbrain
Copy link
Contributor

See #153 for discussion..

@codecov
Copy link

codecov bot commented Nov 15, 2017

Codecov Report

Merging #155 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #155   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines          57     57           
  Branches       13     13           
=====================================
  Hits           57     57

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50299d9...60196af. Read the comment docs.

Copy link
Owner

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Thank you for putting this work in!


They will be available to subprocesses as normal however:

```javascript
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe we could get rid of the two other examples and just use this one. Nobody wants the behavior of the other two examples so they are just noise in the README.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW I have scripts that use the behaviour of the other two examples in a multi-platform Jenkins pipeline so they would have been useful to me.

Copy link
Owner

Choose a reason for hiding this comment

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

Interesting... How is that useful? Do you choose which to run based on the platform you're using? Doesn't that sort of defeat the purpose of using cross-env?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use cross-env to declare env vars across different platforms in a common way high in the calling chain, then specialise as late as possible for paths, commands, switches, error handling and other cross platform incompatibilities.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a better example would be (I haven't tested it, away from work computer):

"parentScript": "cross-env GREET=\"Joe\" npm run childScript",
"childScript": "cross-env echo Hello $GREET"

Basically everywhere you declare or use an env variable, you need to parse that command through cross-env or cross-env-shell, but it's possible to make a full scripts suite multi-platform.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me:

"parentScript": "cross-env GREET=\"Joe\" npm run childScript",
"childScript": "cross-env echo Hello $GREET"

run in cmd.exe on Windows results in:

C:\Users\Me\temp>npm run parentScript

> temp@1.0.0 parentScript C:\Users\Me\temp
> cross-env GREET="Joe" npm run childScript


> temp@1.0.0 childScript C:\Users\Me\temp
> cross-env echo Hello $GREET

Hello %GREET%

Copy link
Owner

Choose a reason for hiding this comment

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

Huh, weird. I'm honestly not too concerned about this because the use case I'm interested in doesn't involve setting env variables in one script and using them in another 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, it needs to be:

"parentScript": "cross-env GREET=\"Joe\" npm run childScript",
"childScript": "cross-env-shell echo Hello $GREET"

I just use cross-env-shell always so I forget the differences between both commands haha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually that might be the update the README needs!

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, let's do it 👍

Though I think that it should be:

"parentScript": "cross-env GREET=\"Joe\" npm run childScript",
"childScript": "cross-env-shell \"echo Hello $GREET\""

@achingbrain
Copy link
Contributor Author

Switching from cross-env to cross-env-shell did indeed do the trick.

@achingbrain achingbrain closed this Dec 7, 2017
achingbrain added a commit to achingbrain/cross-env that referenced this pull request Dec 7, 2017
After discussion on kentcdodds#155 this seems like the better solution.
kentcdodds pushed a commit that referenced this pull request Dec 27, 2017
After discussion on #155 this seems like the better solution.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants