WIP: Issue #13 Sync support

[phillipluther]

Refactor to add sync support for CSS compilation; opening as a WIP request. There's a lot of duplication in both the render and renderSync methods in index.js, as well as in the tests.

Gonna keep playing with this to try and streamline, but initial thoughts/call-outs:

• Added a global SYNC flag to determine whether certain methods (writeFile, compile, etc.) behave syncrhonously or not; this seemed more elegant - though admittedly less explicit - for handling compilation via render or renderSync
• Refactored several of the utility methods to conditionally return promises on whether or not we're in SYNC mode

[marksmccann]

@phillipluther So after reading through your changes I think I have mixed feelings because I can see pros and cons to, as I see it, two primary approaches.

1. Join Sync and Async implementations into each relevant method
2. Create separate implementations for Sync and Async methods

If we go the first route, which you mostly did, I like how a single function can serve dual purposes but I don't like how it requires a prop and I don't like how it adds a condition and that we have to use a more verbose syntax. If we keep with this route I think we should at least take full advantage of it and do something like this (to avoid duplication) –

function renderSync(userOptions) {
     SYNC = true;
     return render(userOptions);
}

On the other hand however, if we went the other route, I like how Sync and Async are completely independent and isolated. I like how we can use the best syntax for both scenarios without a condition or a configuration variable. I also like how the API would reflect popular tools in the industry like path and fs that separate those functions entirely. But, I also don't like the duplication.

So again, conflicted. I think I slightly favor the option 2, but I am curious what are your thoughts on this.

[phillipluther]

RE: separate methods for sync/async VS. a single method with the conditional check ... I feel ya. I'm torn on it, too. I freakin' hate the idea of duplication but am also not keen on the idea of virtually every method having two modes of operation based on some conditional check. There's something impure about that.

I'll keep this PR open, cut a new branch from initial-release and take the other approach. That way we'll have 'em side-by-side. That might make it easier to look at/compare; it'll probably also give us an idea of where we can break things down even further for re-use.

[marksmccann]

First off, thanks for throwing together that other PR so we can compare the two methods. I think really helped me choose a side. I think I am squarely on the "separate methods for sync/async" team and here's why ...

• It's cleaner; each function gets to be it's best self. I think this will lead to greater readability and maintainability.
• As it is, sync/async require different syntax and they cannot share functionality. Combining them into a single function doesn't cut down on logic or duplication, in fact I would argue it makes it worse and more complicated.
• It seems to me the only duplication we are avoiding is in the renderAsync method. The sync/async utility methods are separate implementations either way. It still requires most of the same unit tests. Are the few lines we can save in the renderAsync method worth it?

IMO, the largest argument for combining Sync/async was to avoid duplication. However, it doesn't seem like it really saves that much. Not only that, but the approach makes the rest of our package less pure.

I dunno, if you feel really strongly about the other approach, make you're your case, I can be persuaded. However, for now, I vote we go with "separate methods for sync/async".

[phillipluther]

I concur. I went back and looked at it this morning and all the “duplication” is logic; the a/sync functions are doing the same things but in very different ways.

Separating them feels cleaner. I don’t like how the previous PR has all these double-duty functions that conditionally return a promise.

Even the tests — which were a straight up copy paste — had to be updated from async to not.

Sold. I’ll do some cleanup and charge ahead with distinct methods. Thanks for the sanity check!

---

From: Mark McCann notifications@github.com Sent: Friday, August 16, 2019 5:48:53 AM To: marksmccann/node-sass-extra node-sass-extra@noreply.github.com Cc: Phillip Luther dev@phillipluther.com; Mention mention@noreply.github.com Subject: Re: [marksmccann/node-sass-extra] WIP: Issue #13 Sync support (#31)

First off, thanks for throwing together that other PR so we can compare the two methods. I think really helped me choose a side. I think I am squarely on the "separate methods for sync/async" team and here's why ...

• It's cleaner; each function gets to be it's best self. I think this will lead to greater readability and maintainability.
• As it is, sync/async require different syntax and they cannot share functionality. Combining them into a single function doesn't cut down on logic or duplication, in fact I would argue it makes it worse and more complicated.
• It seems to me the only duplication we are avoiding is in the renderAsync method. The sync/async utility methods are separate implementations either way. It still requires most of the same unit tests. Are the few lines we can save in the renderAsync method worth it?

IMO, the largest argument for combining Sync/async was to avoid duplication. However, it doesn't seem like it really saves that much. Not only that, but the approach makes the rest of our package less pure.

I dunno, if you feel really strongly about the other approach, make you're your case, I can be persuaded. However, for now, I vote we go with "separate methods for sync/async".

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmarksmccann%2Fnode-sass-extra%2Fpull%2F31%3Femail_source%3Dnotifications%26email_token%3DAAKPLB6CXTPIM52W6WXOJCLQE2O3LA5CNFSM4IL3FR7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4OQ2FY%23issuecomment-521997591&data=02%7C01%7C%7Cb87481007bb54e82df1508d72248190d%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637015565358122148&sdata=f%2BUY4Lwb2ksfBJUPftpU3aR0XuPtEq15RY64o19Z3UM%3D&reserved=0, or mute the threadhttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAAKPLB6DZFYJTZUV4PTJSGLQE2O3LANCNFSM4IL3FR7A&data=02%7C01%7C%7Cb87481007bb54e82df1508d72248190d%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637015565358132152&sdata=RtjI3Ns3%2BW%2B5r0uzMTRmZOvafnUltKDKUIVq%2BN2ksh8%3D&reserved=0.

[phillipluther]

Closing in favor of https://github.com/marksmccann/node-sass-extra/pull/32