Skip to content

Conversation

@gdelmas
Copy link
Contributor

@gdelmas gdelmas commented Jun 4, 2019

hi @Quramy,

i made some further changes. mainly to async/await. but also some cleanup and the use of let/const.

for review it is probably worth looking at the changes at each commit one by one, as they are not a lot. the whole diff looks overwhelming and is not reflecting the steps well.

@gdelmas gdelmas force-pushed the cleanup-async-let-const branch from 52baf84 to 48fda1d Compare June 4, 2019 00:23
@gdelmas
Copy link
Contributor Author

gdelmas commented Jun 4, 2019

ahhh, i just noticed utils.promisify is not available in node 6. so test is failing. i will do a custom promisification.

would you generally be open to the changes? i think it is nice as it reduces the lines of code, by keeping the functionality.

@gdelmas
Copy link
Contributor Author

gdelmas commented Jun 9, 2019

hey @Quramy,

what do you think about async/await?

@Quramy
Copy link
Owner

Quramy commented Jun 11, 2019

@gdelmas Sorry for my late reply 🙇

I like async/await syntax too.

And for now unit testing is run in Node.js v6 env, but I think almost dev uses Node >= v8. So we no longer need to support v6.

@Quramy
Copy link
Owner

Quramy commented Jun 11, 2019

You can remove https://github.com/Quramy/typed-css-modules/blob/master/.travis.yml#L9 and use promisify.

This reverts commit 7d4049f
and disables running tests in travis on node 6 and specifies engine support in package.json
@gdelmas
Copy link
Contributor Author

gdelmas commented Jun 11, 2019

hey @Quramy,

great to hear from you and happy you also like async/await better. you are completely right, without node6 support there is less code to maintain. i added a new commit to remove node6 support, where i also added the required node version to package.json so that some developer installing this might get a warning. i kept the commit with node6 support in the history, in case someone needs it and wants to fork.

@Quramy
Copy link
Owner

Quramy commented Jun 13, 2019

Thanks for your great work!!!

@Quramy Quramy merged commit 5e5847e into Quramy:master Jun 13, 2019
@Quramy
Copy link
Owner

Quramy commented Jun 13, 2019

I've published this as v0.5.0 🎉

@gdelmas
Copy link
Contributor Author

gdelmas commented Jun 14, 2019

amazing, thanks @Quramy

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.

2 participants