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

Support for multiple arguments in --before-build #70

Closed
tomek-he-him opened this issue Oct 6, 2016 · 10 comments
Closed

Support for multiple arguments in --before-build #70

tomek-he-him opened this issue Oct 6, 2016 · 10 comments

Comments

@tomek-he-him
Copy link
Collaborator

tomek-he-him commented Oct 6, 2016

We made a conscious decision to roll out an MVP --before-build and wait for feedback from our users. There are two options to take this further:

  • Run the command inside a subshell, like this:

    elm-live --before-build="my-command arg1 'arg 2'"

    This would probably feel pretty familiar for most of us. But this comes at a cost of poorer performance and implicitness (Which shell should we use to run the command? The current shell? A POSIX shell? cmd.exe?).

  • Spawn the command directly, like this:

    elm-live --before-build=[ my-command arg1 'arg 2' ]

    This is more explicit and much faster, but may be a surprising syntax for you guys.

Where should we take this? You can vote below, we’ll be super grateful for your feedback!

@tomek-he-him
Copy link
Collaborator Author

subshell

@tomek-he-him
Copy link
Collaborator Author

direct

@witoldsz
Copy link

direct can be problematic when one would like to use shell specific stuff like pipes, redirects, etc…

@tomek-he-him
Copy link
Collaborator Author

@witoldsz Thanks for sharing your thoughts! It’s true, but I would argue you can put shell-specific syntax in a shell-specific executable.

Accepting shell-specific syntax would introduce the ambiguity I mentioned: which shell do we use to run that? Also, if people want to share elm-live snippets, I’d rather they always work – not only in specific shells.

@witoldsz
Copy link

"scripts" section of package.js (NPM) works fine, cross-platform, so this could work just fine. Also, most common things like pipes and redirects works everywhere (I guess)…

@tomek-he-him
Copy link
Collaborator Author

Yeah, the scripts section works just fine – but many scripts don’t work cross-platform. I’ve worked in a cross-platform team before and npm scripts were one of the biggest pain points to keep portable. Despite following https://github.com/mattdesl/module-best-practices#task-running (I even helped write most of that), we constantly had to jump through hoops to keep it all humming along.

Even in a pure Unix world, you still have different shells people use, and some of them (like fish) are not POSIX-compliant.

As a closing point, npm scripts are designed to run once, so the performance penalty for spawning a subshell is only paid once. With elm-live, that penalty would be paid every time you save a file.

But, let’s wait and see what others say. 🙂 I’ll leave the poll open for now, perhaps your comments would change the game. Thanks again for the valuable feedback!

@tomek-he-him
Copy link
Collaborator Author

Poll results:

support_for_multiple_arguments_in_ --before-build _ _issue__70_ _tomekwi_elm-live

Seems like the direct option wins! I’d love to pick this up as soon as possible, this is a really nice feature! But it’s quite a crazy moment in my life – lots of things going on right now. So if someone needs this feature quickly and wants to jump in on this, I’ll be happy to support. 🙂

@witoldsz
Copy link

witoldsz commented Dec 17, 2016

"direct" can be problematic when one would like to use shell specific stuff like pipes, redirects, etc…

It could be worth mentioning that as a workaround for those who need some kind of shell, the "direct" can be used this way: [ bash -c 'shell commands, scripts, pipes, etc. go here' ].

@tomek-he-him
Copy link
Collaborator Author

Great suggestion @witoldsz!

@wking-io
Copy link
Owner

I have decided to remove both before and after built commands in the new alpha. If this is critical to your development workflow please open an issue with your use case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants