Custom workerfarm, BI-Directional IPC #1105
Conversation
|
Tests pass locally on mac, windows, ubuntu server (with 500mb of ram and 4 cores). |
|
So tests pass on Node 9, but when I tried it on Node 6 I got them to fail. |
|
All tests that pass in master should pass in here now (node 8 & 6) + some that don't pass in master. |
src/utils/localRequire.js
Outdated
| @@ -13,7 +13,10 @@ async function localRequire(name, path, triedInstall = false) { | |||
| resolved = resolve.sync(name, {basedir}); | |||
| } catch (e) { | |||
| if (e.code === 'MODULE_NOT_FOUND' && !triedInstall) { | |||
| await install([name], path); | |||
| await worker.addCall({ | |||
| location: './utils/installPackage.js', | |||
There was a problem hiding this comment.
Seems slightly strange that these locations are relative to the worker, not the current file like other require calls. Maybe it might be more readable if we resolved it here: location: require.resolve('./installPackage')
package.json
Outdated
| @@ -73,6 +72,8 @@ | |||
| "coffeescript": "^2.0.3", | |||
| "cross-env": "^5.1.1", | |||
| "eslint": "^4.13.0", | |||
| "glsl-token-assignments": "^2.0.2", | |||
| "glsl-token-properties": "^1.0.1", | |||
There was a problem hiding this comment.
I think these are just missing from the yarn.lock for some reason. They are already deps of glslify-bundle I think.
src/assets/GLSLAsset.js
Outdated
| @@ -11,6 +11,8 @@ class GLSLAsset extends Asset { | |||
| } | |||
|
|
|||
| async parse() { | |||
| await localRequire('glsl-token-properties', this.name); | |||
| await localRequire('glsl-token-assignments', this.name); | |||
There was a problem hiding this comment.
Same comment as above, these should already get installed as part of glslify-bundle
src/workerfarm/worker.js
Outdated
| @@ -0,0 +1,45 @@ | |||
| require('v8-compile-cache'); | |||
There was a problem hiding this comment.
move this file out of the folder since it is parcel specific
|
really looking forward to a merge of this PR. parcel is hanging on Windows quite often, so this is a godsend! |
src/workerfarm/WorkerFarm.js
Outdated
| forcedKillTime: 100, | ||
| warmWorkers: true, | ||
| useLocalWorker: true, | ||
| workerPath: require.resolve('../worker') |
There was a problem hiding this comment.
This will fail in Node 6. Because babel-register runs out of src instead of lib. Not sure why transpiling doesn't apply on workers though.
I've tried this before writing the pretty hacky way of resolving it
There was a problem hiding this comment.
Ah right. babel-register doesn't run inside the workers.
Codecov Report
@@ Coverage Diff @@
## master #1105 +/- ##
==========================================
- Coverage 88.36% 88.32% -0.05%
==========================================
Files 74 77 +3
Lines 3695 4384 +689
==========================================
+ Hits 3265 3872 +607
- Misses 430 512 +82
Continue to review full report at Codecov.
|
|
Thank you! |
|
Hi everyone! how are you? Is there any breaking change in this PR about how we can define a custom asset? I'm using parcel-plugin-typescript and with the version 1.7.1 of parcel I'm getting this error: fathyb/parcel-plugin-typescript#36 But it seems that everything is ok with the plugin and it was working fine until the version 1.7.0 of parcel. |
|
@tinchoz49 Yes, this PR has a stricter error handling than the previous implementation, therefore unknown ipc communication results into throwing an error. It would also improve performance of parcel-plugin-ts as it doesn't need to start up their own workers |
IPC_CHANNEL_CLOSEDerror at the end of testsparcel watchin 1.5.0 stuck on "Building someFile.js..." #637 Closes Distributed builds #29 Closes Allow adjustment of maxConcurrentCallsPerWorker through env variable #901