-
-
Notifications
You must be signed in to change notification settings - Fork 239
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
use worker-rpc library for inter-process communication #246
Conversation
…aging for minimal code changes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your work! I like the RPC approach - it's a good direction to clean-up the codebase :)
b499451
to
f113f1e
Compare
Okay, everything should be addressed - could you take a second look? :) |
Looks really interesting! My one reservation is that However, it turns out I wonder, given it's already written in TypeScript and released under MIT if we couldn't just use that module in the |
Oh, I should have added that info: @DirtyHairy is actually a colleague of mine - as far as I know it's not maintained because it does what it needs to and there were no changes requested or required in the last few years. We are actively using it in some customer projects and as far as I know he is using it heavily for web worker communication in his atari emulator ( https://github.com/6502ts/6502.ts ), so from my point of view I'd say it is quite mature. As for the failing tests - I'll take a look at that tomorrow in the evening. Guess there were still tests using |
Okay, there was actually a test for WorkResult, which was made obsolete by this PR. Now tests are actually running. Tests are now failing for webpack 2 and 3, I'm on that. |
This test suite is great :) I had missed to migrate one RPC call - now everything should be good :) |
Great! Question: does https://www.npmjs.com/package/@phryneas/experimental-fork-ts-checker-webpack-plugin use worker-rpc? |
Yes, but the version before the fix of yesterday, so the current build will not work on webpack 2/3. |
That'd be awesome - thanks so much! I'm going to switch over my build at work to use this and see how it goes. I'm very much a believer in dogfooding 😉 |
It would be great to get a release out with this in. @piotr-oles do you want to make that happen? Given the experimental version has issues it would be good to ensure that the |
Yes, please release if you want to :) |
Is there a way to do it without raising another PR to update the |
I will create PR with semantic-release integration to make releasing and versioning fully automatic :) |
Nice! 😊❤️ Should I push out v1.0.3 in the meantime or would you rather I leave that so you can test your new mechanism? |
Please release v1.0.3 - I don't want to block you. I will release RC versions on beta channel to test it. |
Should be out shortly https://github.com/Realytics/fork-ts-checker-webpack-plugin/releases/tag/v1.0.3 cc @phryneas |
Over in #231, you suggested
To do that I need a way to do some inter-process communication that does not interfere with the given inter-process communication.
This PR adds worker-rpc so we can send different types of messages between the processes. Also, as it has a nice interface around promises, it acutally makes some existing code more readable.
As it did not want this to be swept under the rug by the myriad of other commits in #231, I created a separate PR for this so you can review it (I will continue using this in #231 as well, though)