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

Remove execa.shell() and execa.shellSync() #219

Merged
merged 8 commits into from
May 8, 2019

Conversation

ehmicky
Copy link
Collaborator

@ehmicky ehmicky commented May 7, 2019

Fixes #211

Since the next version is a major release, I thought this would be the perfect time to directly remove those methods instead of deprecating them.

readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@ehmicky
Copy link
Collaborator Author

ehmicky commented May 7, 2019

Fixed according to the code review.

readme.md Show resolved Hide resolved
@ehmicky
Copy link
Collaborator Author

ehmicky commented May 8, 2019

Fixed.

index.d.ts Outdated
@@ -65,6 +65,7 @@ declare namespace execa {

/**
If `true`, runs `command` inside of a shell. Uses `/bin/sh` on UNIX and `cmd.exe` on Windows. A different shell can be specified as a string. The shell should understand the `-c` switch on UNIX or `/d /s /c` on Windows.
We recommend against using this option since it is not cross-platform, slower and unsafe.
Copy link
Owner

Choose a reason for hiding this comment

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

The TS text should be exactly what's in the readme. Easier to keep them in sync that way.

@sindresorhus
Copy link
Owner

Can you fix the merge conflict?

@ehmicky
Copy link
Collaborator Author

ehmicky commented May 8, 2019

Fixed

@kopax
Copy link

kopax commented Sep 26, 2019

Too bad you are not showing any concret example of before/after of this breaking change.

@ehmicky
Copy link
Collaborator Author

ehmicky commented Sep 26, 2019

Hi @kopax,

You might get some extra information in this Medium article.

Generally speaking:
Before: execa.shell(command) and execa.shellSync(command)
After: execa(command, {shell: true}) and execa.sync(command, {shell: true})

@kopax
Copy link

kopax commented Sep 26, 2019

Thanks, I finally understood by myself, but that's always annoying to have to read the whole documentation to find how the breaking change is affecting our code. I recommend to always have a concrete migration example for all of them

@ehmicky
Copy link
Collaborator Author

ehmicky commented Sep 26, 2019

The release notes show:

### Breaking changes
- Remove `execa.shell()` and `execa.shellSync()`. The [`shell option`](https://github.com/sindresorhus/execa#shell) should be used instead.

The shell option includes a link to its documentation, I think it should be a quite clear migration path. I agree that an explicit before/after example would have been better, but the release notes weren't too cryptic either.

@kopax
Copy link

kopax commented Sep 26, 2019

I got through this but my advice is still valid, that's just general advice so people in general don't have to go through as I did.

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.

Deprecate execa.shell() in favor of execa(..., { shell: true })
3 participants