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

cleanup lib & utils, etc. #15

Closed
brodybits opened this issue Apr 9, 2019 · 10 comments · Fixed by #150
Closed

cleanup lib & utils, etc. #15

brodybits opened this issue Apr 9, 2019 · 10 comments · Fixed by #150

Comments

@brodybits
Copy link
Owner

brodybits commented Apr 9, 2019

I suspect most of the sources in utils could be replaced by existing npm packages. Maybe a limited number of new utility packages to replace the rest.

related to: #14 - move some JavaScript files into lib subdirectory

P.S. My goal is to get rid of the utils subdirectory, if possible.

P.S. 2: Title shows "Fixed by #150" but this cleanup was actually done through multiple changes.

@brodybits
Copy link
Owner Author

Here are my findings from a quick look through what is now in lib/utils:

  • use the Promise-based interface of fs-extra calls instead of createFile.js & createFolder.js
  • replace implementation in exec.js with use of execa package; also replace use of any child_process calls with execa calls in lib.js
  • I think that hasPrefix.js and isUpperCase.js can go away
  • implementation in npmAddScriptSync.js which updates scripts in package.json can be replaced by using one of the following utilities:

Synchronous file & process exec calls should be avoid whenever possible.

I think it would be ideal to use await and async instead of Promise chains (#39).

@dsyne
Copy link
Contributor

dsyne commented Jul 22, 2019

Hey sorry, been a bit busy but am looking at this now, have removed most of the utils. Am currently removing promises for async/await

@brodybits
Copy link
Owner Author

Thanks @dsyne for your work so far on this, no rush on my part.

I already saw your remove-utils branch, and I used the first couple changes in WIP PR #52 to start adding automatic testing (#40). I have also factored out lib/utils/exec.js in the same PR, now with a mockable API that I can use for automatic testing.

I would actually like to hold off on changes such as integrating execa and using await & async until the automatic testing is ready and integrated. I will also take a look into mocking and testing the of the example package JSON functionality.

I will likely hide some of our conversation in #28 as resolved.

@dsyne
Copy link
Contributor

dsyne commented Jul 23, 2019

Ok, no worries. Shall i park the rest of the work on that branch for now then? I'm happy to pick up another issue, maybe #37 and #5 unless anything else is more pressing

@brodybits brodybits mentioned this issue Jul 23, 2019
@brodybits brodybits changed the title Cleanup utils Cleanup lib & utils Jul 23, 2019
@brodybits
Copy link
Owner Author

Hi @dsyne, I just updated the title to hopefully match of the scope of our ongoing discussion. My response should be coming with some ideas in the next 6-12 hours or so. Thanks for your help so far.

@brodybits
Copy link
Owner Author

brodybits commented Jul 25, 2019

My apologies for the delay, I am still in the middle of a few things so you may as well park the work for now. I hope to send my response soon. Proposals, comments, and discussions are always welcome but beware that I will likely make some major changes to the JavaScript in the next 1-2 days or so.

@dsyne
Copy link
Contributor

dsyne commented Jul 31, 2019

No problem at all, i've been away for a few days and am just catching up with things at work. I'll happily jump back on when you have things sorted so i'll keep an eye out. 👌

@brodybits
Copy link
Owner Author

FYI you will likely see some more updates before you see my response. I hope to finish these soon, no promises yet. Another thing is that I may start to use this package to promote some of my services. I may consider promoting other people in the future, no promises though.

In general, critical feedback on PRs and other proposed ideas would always be helpful. Also for draft and WIP PRs.

@brodybits brodybits changed the title Cleanup lib & utils cleanup lib & utils, etc. Sep 12, 2019
@brodybits
Copy link
Owner Author

Hi @dsyne, my apologies for continued lack of response. I am still pretty busy with some major updates, and in parallel with some other projects and commitments.

It may be a very long time before I get organized enough to get into formal delegation, and this may never happen. I am thinking it would be more helpful if you can contribute some feedback on issues, proposals, and discussions as they come up, and maybe take a look at some "help wanted" issues. And any other proposals you can think of would be welcome for discussion and consideration. You may want to look at some recent contributions by @dlowder-salesforce for some inspiration.

And I would like to thank you for recommending this project in facebook/react-native-website#1118.

@brodybits
Copy link
Owner Author

Remaining artifact was factored out of lib/utils so this task should be finished now.

Thanks @dsyne for your offers to help. As I tried to explain before, your input and contributions would be appreciated anytime.

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 a pull request may close this issue.

2 participants