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

use worker-rpc library for inter-process communication #246

Merged
merged 6 commits into from
Apr 17, 2019

Conversation

phryneas
Copy link
Contributor

Over in #231, you suggested

I wonder if there's a way to tweak the checker to expose the checker purely for the purposes of testing... That could be potentially useful - it wouldn't need to be something we exposed always; we could have a "test" mode or something... Ideas!

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)

Copy link
Collaborator

@piotr-oles piotr-oles left a 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 :)

src/cluster.ts Outdated Show resolved Hide resolved
src/cluster.ts Outdated Show resolved Hide resolved
src/service.ts Outdated Show resolved Hide resolved
@phryneas phryneas force-pushed the worker-rpc branch 4 times, most recently from b499451 to f113f1e Compare April 14, 2019 20:13
@phryneas
Copy link
Contributor Author

Okay, everything should be addressed - could you take a second look? :)

@johnnyreilly
Copy link
Member

johnnyreilly commented Apr 15, 2019

Looks really interesting!

My one reservation is that worker-rpc doesn't seem to be an actively maintained project and no-one is actively using it. (40 downloads a week on npm) This makes me hesitant to create a dependency on it.

However, it turns out worker-rpc is a pretty simple codebase; it pretty much amounts to this file: https://github.com/DirtyHairy/worker-rpc?files=1

I wonder, given it's already written in TypeScript and released under MIT if we couldn't just use that module in the fork-ts-checker-webpack-plugin codebase directly? I'd be happy to have a comment in the codebase that reflects that. And I'd be happy (if @DirtyHairy would like it) to have them listed as a contributor.

@phryneas
Copy link
Contributor Author

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 WorkResult.ts, even though the code did not reference it - and as I had a compiled version still sitting in the lib folder, my local tests did not fail.

@DirtyHairy
Copy link

DirtyHairy commented Apr 15, 2019

What @phryneas says: both worker-rpc and microevent.ts (on which it depends) are maintained; they just haven't required much maintenance since I published them 😏 Both are heavily used in 6502.ts and are covered with tests. I trust them enough to use them in customer projects.

@phryneas
Copy link
Contributor Author

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.

@phryneas
Copy link
Contributor Author

This test suite is great :) I had missed to migrate one RPC call - now everything should be good :)

@johnnyreilly
Copy link
Member

@phryneas
Copy link
Contributor Author

Yes, but the version before the fix of yesterday, so the current build will not work on webpack 2/3.
I can merge it over and create a new build this evening.

@johnnyreilly
Copy link
Member

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 😉

@piotr-oles piotr-oles merged commit 4817f01 into TypeStrong:master Apr 17, 2019
@johnnyreilly
Copy link
Member

johnnyreilly commented Apr 17, 2019

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 worker-rpc tweaks themselves are stable. I'm pretty optimistic, so let's ship!

@piotr-oles
Copy link
Collaborator

Yes, please release if you want to :)

@johnnyreilly
Copy link
Member

Is there a way to do it without raising another PR to update the package.json... I'm guessing not?

@piotr-oles
Copy link
Collaborator

I will create PR with semantic-release integration to make releasing and versioning fully automatic :)

@johnnyreilly
Copy link
Member

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?

@piotr-oles
Copy link
Collaborator

Please release v1.0.3 - I don't want to block you. I will release RC versions on beta channel to test it.

@johnnyreilly
Copy link
Member

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

Successfully merging this pull request may close these issues.

4 participants