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

perf: do not spawn new process for running webpack #1741

Merged
merged 9 commits into from
Aug 17, 2020
Merged

Conversation

anshumanv
Copy link
Member

What kind of change does this PR introduce?

fix

Did you add tests for your changes?
Not yet

If relevant, did you update the documentation?
No

Summary
Do not spawn new process for running the CLI

Does this PR introduce a breaking change?
Shouldn't

Other information

@alexander-akait
Copy link
Member

Do you need help with tests?

@snitin315
Copy link
Member

Yes, I'm helping with the node flags support. I will push changes soon.

@snitin315 snitin315 changed the title Fix: do not spawn new process for CLI chore: remove execa Aug 13, 2020
@snitin315
Copy link
Member

Using NODE_OPTIONS gives desired behavior with webpack@next only and valid node flags but if we use invalid node flags it throws the following error -
Screenshot at 2020-08-13 18-39-33

@alexander-akait
Copy link
Member

I think we should remove --node-args in favor NODE_OPTIONS, because all tools uses NODE_OPTIONS, no one implement --node-args

@anshumanv
Copy link
Member Author

I think we should remove --node-args in favor NODE_OPTIONS, because all tools uses NODE_OPTIONS, no one implement --node-args

Yeah we can remove, also doing unnecessary parsing

@alexander-akait
Copy link
Member

Let's do it here 👍

@anshumanv
Copy link
Member Author

Okay, on it

@alexander-akait
Copy link
Member

alexander-akait commented Aug 13, 2020

Here we improve starting webpack around 50-200ms on different computers (already tested), it is very good improvements

@anshumanv
Copy link
Member Author

Here we improve starting webpack around 50-200ms on difference computer (already tested), it is very good improvements

Thanks for the benchmark!

@anshumanv anshumanv changed the title chore: remove execa perf: do not spawn new process for running webpack Aug 13, 2020
@anshumanv anshumanv marked this pull request as ready for review August 13, 2020 15:55
@anshumanv anshumanv requested a review from a team as a code owner August 13, 2020 15:55
@anshumanv
Copy link
Member Author

CI freeze 😞

@anshumanv anshumanv closed this Aug 13, 2020
@anshumanv anshumanv reopened this Aug 13, 2020
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Great work!

@alexander-akait
Copy link
Member

/cc @webpack/cli-team need review

Copy link
Member

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

💯

@anshumanv
Copy link
Member Author

Something wrong with commitlint 😞

@anshumanv anshumanv closed this Aug 14, 2020
@anshumanv anshumanv reopened this Aug 14, 2020
@alexander-akait
Copy link
Member

@anshumanv Don't worry about commitlint I will fix commit message before merge, anyway we need look why it is broken

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

Successfully merging this pull request may close these issues.

None yet

6 participants