Skip to content
This repository was archived by the owner on May 1, 2020. It is now read-only.

feat(createWorker): pass argv and config_argv to spawned processes #487

Merged
merged 1 commit into from
Jan 28, 2017

Conversation

alan-agius4
Copy link
Contributor

@alan-agius4 alan-agius4 commented Nov 29, 2016

Short description of what this resolves:

In my cleancss.config.js and uglifyjs.config.js, I am dependent on npm args. However the worker-client is being spawned as another process, which is causing the args and npm config argv not to be passed.

Changes proposed in this pull request:

  • pass the argv

Fixes: #463

@alan-agius4
Copy link
Contributor Author

@danbucholtz new PR here.

@danbucholtz
Copy link
Contributor

@alan-agius4,

What is process.env.npm_config_argv here? Couldn't you just pass the process.argv?

Thanks,
Dan

@alan-agius4
Copy link
Contributor Author

Not really as args flag get 'lost'. Example if i have npm scripts that
calls another one.

Another alternative would be to get the args via
JSON.parse(process.env.npm_config_argv).original and pass them as a
parameter merged with process.args

@alan-agius4
Copy link
Contributor Author

alan-agius4 commented Nov 29, 2016

The alternative would be something like what you are doing here: https://github.com/driftyco/ionic-app-scripts/blob/13d97461c2410f9694ea866e6250d12680c89012/bin/ionic-app-scripts.js

Let me know which one you prefer

@alan-agius4
Copy link
Contributor Author

@danbucholtz any update please on which is the prefered approach?

@alan-agius4 alan-agius4 reopened this Nov 30, 2016
@danbucholtz
Copy link
Contributor

I will take a look at this. Sorry, been focused 100% on some AoT changes.

Thanks,
Dan

@danbucholtz danbucholtz self-assigned this Dec 1, 2016
@danbucholtz
Copy link
Contributor

It's gonna be another week or so. Just giving you a heads up. I am 100% focused on getting what we presently have into a shippable state for a 1.0.0.

Thanks,
Dan

@danbucholtz
Copy link
Contributor

@alan-agius4,

Can you write some tests to validate this? We are putting a bigger focus on unit tests with all new functionality.

Thanks,
Dan

@alan-agius4
Copy link
Contributor Author

alan-agius4 commented Dec 17, 2016 via email

@danbucholtz
Copy link
Contributor

Sounds good, no hurry @alan-agius4. We appreciate your help!

Thanks,
Dan

@alan-agius4
Copy link
Contributor Author

Hi @danbucholtz,

I am trying to test the forked process however, I am finding it a bit hard as they have an easy way to access their argv, is not within an executed scripts. Any tips please?

@danbucholtz
Copy link
Contributor

@alan-agius4,

Not sure 😆 . Mock as much as needed.

Thanks,
Dan

@danbucholtz danbucholtz merged commit 02dfff8 into ionic-team:master Jan 28, 2017
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.

2 participants