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

Improvements #35

Merged
merged 5 commits into from
Aug 13, 2020
Merged

Improvements #35

merged 5 commits into from
Aug 13, 2020

Conversation

eimanip
Copy link
Contributor

@eimanip eimanip commented Aug 12, 2020

Hi,

First of all, thanks for the great script!

I made a couple of improvements in the script. The changes are backward-compatible. Here are the changes:

  • Use path.join() instead of string concatenation for making file names: Using string concatenation is unsafe. Also, it made the output file to have analytics.js path file to be like ".//analytics.js". It would fix this.
  • Implement the ability to change the name of the gtag.js file name in CLI: I wanted the ability to specify a different file name for the output file instead of google-analytics-local.js
  • Remove tmp directory: It's already specified in the .gitignore file, however it was included in the repo.
  • Automatically make necessary directories while saving the file: Using fs.mkdir, we can make sure that the specified folder is created and exists without prior need to do create it manually.

Let me know what you think.
Thanks

P.S. I made some other changes based on this branch to be able to specify the output directory. Currently, it's writing the folder path in the script file, which is something that I don't want. Because the changes are not backward-compatible, I didn't include them in this pull request. You can find these changes here.

@scriptex scriptex self-assigned this Aug 12, 2020
@scriptex scriptex added the enhancement New feature or request label Aug 12, 2020
@scriptex
Copy link
Owner

Thanks, @eimanip ! I will take a look as soon as I can.

index.js Outdated Show resolved Hide resolved
Copy link
Owner

@scriptex scriptex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides a one minor thing (yes, I know ...) everything looks good :)

@scriptex scriptex merged commit bb16608 into scriptex:master Aug 13, 2020
@scriptex
Copy link
Owner

The improvements are available on NPM and Github 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants