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

Allow setting --max-workers / other publish options #32

Open
motiz88 opened this issue Mar 25, 2018 · 5 comments
Open

Allow setting --max-workers / other publish options #32

motiz88 opened this issue Mar 25, 2018 · 5 comments

Comments

@motiz88
Copy link

motiz88 commented Mar 25, 2018

Background

I'm encountering a similar issue to #21 (only it's on Travis rather than CircleCI). sshing into the build worker and tweaking appr/index.js, I am able to get it to publish using exp publish --max-workers=1 (introduced in exp v50.0.0). (I'm going to try using a quick fork of appr to validate that this completely solves the issue, will report back)

Feature suggestions

What do you think of having a generic way to pass options to exp publish through appr? Or at least a way to pass --max-workers through? Or even just defaulting to --max-workers=1?

@motiz88
Copy link
Author

motiz88 commented Mar 25, 2018

I'm going to try using a quick fork of appr to validate that this completely solves the issue, will report back

Update: Yep! Can't share my actual build log as it's a private project, but https://github.com/motiz88/appr/tree/limit-max-workers did the trick. I have seen Travis occasionally barf on builds that spawned "too many" processes (Jest used to suffer from this I think), so I guess this is one of those cases.

Also, I'm pretty sure this isn't a false positive; my build failures without this patch have been very consistent, across CI runs, across versions of exp etc.

So, now that it's fully justified - back to the feature discussion 😄 Happy to put together a PR with whatever API is agreed here.

@jevakallio
Copy link
Contributor

@motiz88 this is good.

API-wise, I think we should just whitelist any args that Appr needs itself (currently none) and pass forward any remaining --args to the exp process (like yarn run does nowadays).

Alternatively we can whitelist all the known exp flags and do the reverse, but that would require us to keep track of new (and possibly undocumented) exp flags.

What's your preference @motiz88?

@motiz88
Copy link
Author

motiz88 commented Mar 26, 2018

One subtle issue with a generic solution is that appr invokes two exp commands (login and publish), so forwarding to publish is not necessarily all anyone would need. Not sure what I'd prefer here, as a user, other than a "do what I mean" solution that does effectively have to know which argument goes with which command.

@jevakallio
Copy link
Contributor

Right you are. Let me noodle about this a little bit. If you have a concrete suggestion I'm happy to hear it.

Some ideas:

  • Prefix arbitrary arguments with --exp-login-* and --exp-publish-*
  • Environment variables for known args (we already use these a lot)
  • .apprrc (err... maybe not this one)

@motiz88
Copy link
Author

motiz88 commented Mar 26, 2018

I like --exp-login-* and --exp-publish-*, but I believe parsing these with parameters (which can be space-separated etc) would still require appr to know exp's options schema.

Maybe EXP_PUBLISH_ARGS and EXP_LOGIN_ARGS env vars that get interpolated into the command? (Though thinking about the correct handling of spaces and escaped strings here makes my head hurt)

Thing is, all the generic solutions seem to have edge cases that probably exceed the complexity budget here. I'm kinda leaning towards whitelisting options one-by-one (either --exp-cmd-prefixed or not) as specific use cases for them arise in the wild - which will likely be few in practice.

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

No branches or pull requests

2 participants