#1 #7

Open
wants to merge 13 commits into
from

Projects

None yet

3 participants

@robinvenneman
Collaborator

Replaces readFileSync with async readFile

@phated
Member
phated commented Nov 28, 2016

I'm having a hard time reviewing this with the github diff tool. I might need to pull it down locally and read just the end result.

@robinvenneman
Collaborator

If it's too complex like this it could be refactored using promises or even async/await, as third-party modules for backwards compatibility.

@phated
Member
phated commented Dec 26, 2016

I don't think it's too complex, I just haven't had a chance to do a thorough review due to the github diff being bad.

Reminder: we want to keep ES5-compat so async/await would be out of the question.

@robinvenneman
Collaborator

I see, didn't realise it was the diff, thought the code was not very clear. As far as compatibility I know this should be ES5 compatible that's why I was thinking about using Bluebird or async-await but I avoided adding additional dependencies for now. I do think it gets harder to reason about like this though.

@contra contra was assigned by phated Dec 30, 2016
@phated
Member
phated commented Dec 30, 2016 edited

Assigned this to @contra incase he has time to review/make suggestions/help this week

@contra
Member
contra commented Dec 30, 2016 edited

@robinvenneman Have you thought about using the async module (or smaller similar modules) to orchestrate the async stuff vs. doing it all by hand? I think it would clean this up a lot.

sidenote: index.js needs to be broken apart big time, not in this PR though. if you're interested, we'd love your help.

@robinvenneman
Collaborator

@contra yes I did consider async, even mentioned it on #1 and then tried it without first. There's just so many libraries out there as well. If needed I can rework it in a few days.

And I agree about the index file, it should definitely be refactored. I'd be glad to help.

@robinvenneman
Collaborator

Refactored the main methods with async.waterfall. It's a little cleaner, @contra what do you think?

@contra
contra requested changes Jan 8, 2017 edited View changes

Code needs a little cleanup, thanks for putting in the time!

index.js
+ sourceMap = generator.toJSON();
+ } else if (fileType === '.css') {
+ var ast = css.parse(fileContent, { silent: true });
+ var registerTokens = function (ast) {
@contra
contra Jan 8, 2017 Member

Defining functions inside of if blocks is kind of sketch, should break these out to the highest scope wherever possible

index.js
+ }
+ for (var key in ast) {
+ if (key !== 'position') {
+ if (Object.prototype.toString.call(ast[key]) === '[object Object]') {
@contra
contra Jan 8, 2017 Member

Should use a cleaner way of doing this

index.js
+ if (key !== 'position') {
+ if (Object.prototype.toString.call(ast[key]) === '[object Object]') {
+ registerTokens(ast[key]);
+ } else if (ast[key].constructor === Array) {
@contra
contra Jan 8, 2017 Member

Use Array.isArray

index.js
+ if (Object.prototype.toString.call(ast[key]) === '[object Object]') {
+ registerTokens(ast[key]);
+ } else if (ast[key].constructor === Array) {
+ for (var i = 0; i < ast[key].length; i++) {
@contra
contra Jan 8, 2017 Member

Can you use functional iterators instead of forEach loops? Maps cleaner/less code.

This specific case would be one line: ast[key].forEach(registerTokens)

@robinvenneman
robinvenneman Jan 8, 2017 Collaborator

Ok, I see what you mean, it's still not very functional with the side effects (generator, source) but used an IIFE to call the registerTokens first... Might need some more work to make it more pure.

index.js
+ for (var key in ast) {
+ if (key === 'position' || !ast[key]) {
+ break;
+ } else {
@contra
contra Jan 8, 2017 Member

else not needed since it would break if the first condition is true

index.js
+ break;
+ } else {
+ if (ast[key] instanceof Array) {
+ for (var i = 0; i < ast[key].length; i++) {
@contra
contra Jan 8, 2017 Member

ast[key].forEach() instead of for loops

@robinvenneman
robinvenneman Jan 8, 2017 edited Collaborator

yeah this was fixed in the next commit, well spotted the unneeded else though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment