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

Speed up `bin/webpack*` executables by dropping `yarn run` overhead #638

Merged
merged 1 commit into from Aug 12, 2017

Conversation

Projects
None yet
3 participants
@javan
Member

javan commented Aug 12, 2017

We were already doing this on Windows only (#584). Just do it.

Before

$ time bin/webpack
yarn run v0.27.5
$ "/Users/javan/Desktop/myapp/node_modules/.bin/webpack" "--config" "/Users/javan/Desktop/myapp/config/webpack/development.js"
Hash: 9a50a6710c2068693890
Version: webpack 3.5.4
Time: 1005ms
         Asset     Size  Chunks                    Chunk Names
hello_react.js  2.05 MB       0  [emitted]  [big]  hello_react
  [82] ./app/javascript/packs/hello_react.jsx 759 bytes {0} [built]
    + 184 hidden modules
Done in 1.77s.

real	0m2.309s
user	0m2.295s
sys	0m0.210s

After

$ time bin/webpack
Hash: 9a50a6710c2068693890
Version: webpack 3.5.4
Time: 958ms
         Asset     Size  Chunks                    Chunk Names
hello_react.js  2.05 MB       0  [emitted]  [big]  hello_react
  [82] ./app/javascript/packs/hello_react.jsx 759 bytes {0} [built]
    + 184 hidden modules

real	0m1.803s
user	0m1.827s
sys	0m0.169s

@javan javan requested review from gauravtiwari and dhh Aug 12, 2017

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Aug 12, 2017

Member

I thought it wasn't safe to go straight to NODE_MODULES/.bin by yourself?

Member

dhh commented Aug 12, 2017

I thought it wasn't safe to go straight to NODE_MODULES/.bin by yourself?

@gauravtiwari

This comment has been minimized.

Show comment
Hide comment
@gauravtiwari

gauravtiwari Aug 12, 2017

Collaborator

@dhh Should be alright however sometimes binaries might get corrupted specially after yarn install (yarnpkg/yarn#1981), probably fixed now. Another thing yarn run does is multiple lookups before bailing out but we are already using the binaries directly on windows so may be do the same for others.

@javan Have you come across this problem when using binaries directly? #364, sometimes the process isn't get killed

Collaborator

gauravtiwari commented Aug 12, 2017

@dhh Should be alright however sometimes binaries might get corrupted specially after yarn install (yarnpkg/yarn#1981), probably fixed now. Another thing yarn run does is multiple lookups before bailing out but we are already using the binaries directly on windows so may be do the same for others.

@javan Have you come across this problem when using binaries directly? #364, sometimes the process isn't get killed

@javan

This comment has been minimized.

Show comment
Hide comment
@javan

javan Aug 12, 2017

Member

Executing node_modules/.bin/* is safe and is what yarn run eventually does in our case. yarn run can do additional things like execute "scripts" defined in package.json, but we don't need any of that.

@gauravtiwari, I can't reproduce #364:

Run foreman start

Start a new terminal and run foreman stop, and ps aux | grep webpacker will sometimes show the child process continue in the background.

foreman stop doesn't appear to even be a valid foreman command and stopping foreman "normally" with ctrl+c works as expected.

Member

javan commented Aug 12, 2017

Executing node_modules/.bin/* is safe and is what yarn run eventually does in our case. yarn run can do additional things like execute "scripts" defined in package.json, but we don't need any of that.

@gauravtiwari, I can't reproduce #364:

Run foreman start

Start a new terminal and run foreman stop, and ps aux | grep webpacker will sometimes show the child process continue in the background.

foreman stop doesn't appear to even be a valid foreman command and stopping foreman "normally" with ctrl+c works as expected.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Aug 12, 2017

Member

We don't need foreman anymore now that we're proxing to the dev server.

Member

dhh commented Aug 12, 2017

We don't need foreman anymore now that we're proxing to the dev server.

@dhh dhh merged commit 732a70c into master Aug 12, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@dhh dhh deleted the exec-bin-directly branch Aug 12, 2017

@gauravtiwari

This comment has been minimized.

Show comment
Hide comment
@gauravtiwari

gauravtiwari Aug 12, 2017

Collaborator

@javan Yeah with foreman this isn't a problem but I guess it's happening when we run it directly.

Collaborator

gauravtiwari commented Aug 12, 2017

@javan Yeah with foreman this isn't a problem but I guess it's happening when we run it directly.

@renchap renchap referenced this pull request Sep 6, 2017

Closed

yarn run? #765

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