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

Simplify code base by using deasync #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Simplify code base by using deasync #2

wants to merge 1 commit into from

Conversation

wbyoung
Copy link
Owner

@wbyoung wbyoung commented Jan 16, 2017

This addresses the client-server concerns from #1 by using deasync to completely remove the client-server setup. I'm a little wary about how deasync achieves its goal, but have looked a fair amount into possible improvements including replacing use of some private process APIs. To that end, I've opened abbr/deasync#75.

This is, however, a build-only tool & should never really be used in production. So if for some reason something goes wrong because of this private API use, it's far less devastating in during a build than in production.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 6a6856d on deasync into 209ce23 on master.

@gajus
Copy link

gajus commented May 8, 2017

I was looking into deasync too to solve an equivalent problem. gajus/babel-plugin-react-css-modules#83

I'd be curious to know what are the performance implications. As you said, this is a built-time only tool, though I wouldn't like my build tool to consume 100% CPU when idle (which, correct me if I am wrong, would be the result of consumingdeasync).

@wbyoung
Copy link
Owner Author

wbyoung commented May 8, 2017

@gajus no, it wouldn't consume 100% CPU while idle. First, deasync is a busy loop only in the sense that it while (!done) loops, but it's running the event loop during each check, so it's actually really only busy waiting while other things are going on. Second, and more relevant to your question, this would only run when the plugin is processing an import statement — only when you're compiling your code w/ Babel.

@chrisngobanh
Copy link
Contributor

What is blocking this PR? I hope to see this change implemented because the plugin seems unnecessarily bulky with the server-client set up.

@wbyoung
Copy link
Owner Author

wbyoung commented Aug 4, 2017

@chrisngobanh discussion & testing really. I'm a little nervous about it because I'm not crazy about hacking at Node.js the way that deasync is doing in order to get things working. It's not the most future-proof solution, and requires deep understanding of v8/Node.js in order to write those few little lines of C++ code.

Honestly, the fluctuations in how events work between recent versions of Node (with some focus on Promise related features) has been decently large. Even using NaN doesn't future-proof it because it's running the event loop manually.

Things like abbr/deasync#82 concern me as well… just having that in the dependency chain and having to ensure that the project re-builds as required for Node.js releases.

I'm just struggling with how worthwhile it'd be.

Honestly, abstracting the client/server piece into a standalone project may simplify the code base nice enough.

Thoughts?

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